Fix distributed calculator tutorial Go docker build #1058
Conversation
Signed-off-by: Anton Troshin <[email protected]>
There was a problem hiding this comment.
Hey - thank you @antontroshin I noticed build is broken and then learned you PR'd a fix. appreciate. Pls see comments and if you agree (and would be willing to test compat).
| @@ -1,8 +1,7 @@ | |||
| #first stage - builder | |||
| FROM golang:1.15-buster as builder | |||
| FROM golang:1.19-buster as builder | |||
There was a problem hiding this comment.
| FROM golang:1.19-buster as builder | |
| FROM golang:1.22-bullseye as builder |
What if we go straight to supported version 1.22 on bullseye so long as there's no breaking changes? Feels like being on latest supported is what we want if we're changing this.
There was a problem hiding this comment.
Yes, I fully agree. I was playing safe with 1.19.
There was a problem hiding this comment.
Ok since it's testing ok, let's go with the 1.22, bullseye and bookworm changes. Are you ok with updating your PR for this?
There was a problem hiding this comment.
Used the same bookworm version for both build and run containers.
| WORKDIR /dir | ||
| COPY app.go . | ||
| RUN go get -d -v | ||
| COPY app.go go.mod go.sum ./ | ||
| RUN CGO_ENABLED=0 GOOS=linux go build -a -installsuffix cgo -o app . | ||
| #second stage | ||
| FROM debian:buster-slim |
There was a problem hiding this comment.
| FROM debian:buster-slim | |
| FROM debian:bookworm-slim |
Similar idea. how about we do bookworm instead so we're on latest supported/patched?
There was a problem hiding this comment.
Note I tested your suggestion and my suggested changes and both work at runtime and are compatible.
There was a problem hiding this comment.
Great, glad to hear it worked. Thanks for testing it.
… tools chains (C#, Node, Python, React). Go handled by dapr#1058 Signed-off-by: Paul Yuknewicz <[email protected]>
…for dist calculator already fixed by dapr#1058 Signed-off-by: Paul Yuknewicz <[email protected]>
|
Note, I followed the good work here in a companion PR #1062 so all dockerfiles are current and use supported bases. |
Signed-off-by: Anton Troshin <[email protected]>
|
@antontroshin ready to merge? The failures I see above are unrelated to this change so those arent blocking. I like how you consolidated on bookworm. |
|
No PR triggers the Build action so we'll basically need to test in prod at this point.. |
|
TBH a little bit concerned about the "Validate Go quickstarts on ubuntu-latest" failure https://github.com/dapr/quickstarts/actions/runs/10079513721/job/27868435196 |
I looked at it. It looks like a timing cold start issue. But it has zero dependencies on this change - this dockerfile is used by tutorial only. The Go quickstart does not use docker. So I say we roll the dice. |
…tClient) (#1062) * Updating gitignore to exclude out/ from dotnet publish Signed-off-by: Paul Yuknewicz <[email protected]> * Fixing build.yaml github action workflow with modern dockerfiles. go for dist calculator already fixed by #1058 Signed-off-by: Paul Yuknewicz <[email protected]> * Updating csharp dockerfile with correct entry point, and do not explicitly expose port Signed-off-by: Paul Yuknewicz <[email protected]> --------- Signed-off-by: Paul Yuknewicz <[email protected]>
Description
Go example was using old Go image (1.15) for newer Go in
go.mod1.19.Updated the image version and included
go.modandgo.sumIssue reference
We strive to have all PRs being opened based on an issue, where the problem or feature have been discussed prior to implementation.
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: