Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix distributed calculator tutorial Go docker build #1058

Merged
merged 2 commits into from
Jul 24, 2024

Conversation

antontroshin
Copy link
Contributor

Description

Go example was using old Go image (1.15) for newer Go in go.mod 1.19.
Updated the image version and included go.mod and go.sum

Issue 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:

  • The quickstart code compiles correctly
  • You've tested new builds of the quickstart if you changed quickstart code
  • You've updated the quickstart's README if necessary
  • If you have changed the steps for a quickstart be sure that you have updated the automated validation accordingly. All of our quickstarts have annotations that allow them to be executed automatically as code. For more information see mechanical-markdown. For user guide with examples see Examples.

Signed-off-by: Anton Troshin <anton@diagrid.io>
msfussell
msfussell previously approved these changes Jul 24, 2024
Copy link
Contributor

@paulyuk paulyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I fully agree. I was playing safe with 1.19.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
FROM debian:buster-slim
FROM debian:bookworm-slim

Similar idea. how about we do bookworm instead so we're on latest supported/patched?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note I tested your suggestion and my suggested changes and both work at runtime and are compatible.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, glad to hear it worked. Thanks for testing it.

paulyuk added a commit to paulyuk/quickstarts that referenced this pull request Jul 24, 2024
… tools chains (C#, Node, Python, React). Go handled by dapr#1058

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
paulyuk added a commit to paulyuk/quickstarts that referenced this pull request Jul 24, 2024
…for dist calculator already fixed by dapr#1058

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
@paulyuk
Copy link
Contributor

paulyuk commented Jul 24, 2024

Note, I followed the good work here in a companion PR #1062 so all dockerfiles are current and use supported bases.

@paulyuk paulyuk added this to the 1.14 milestone Jul 24, 2024
@paulyuk paulyuk added area/tutorials build bug Something isn't working P0 labels Jul 24, 2024
Signed-off-by: Anton Troshin <anton@diagrid.io>
@paulyuk
Copy link
Contributor

paulyuk commented Jul 24, 2024

@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.

@paulyuk
Copy link
Contributor

paulyuk commented Jul 24, 2024

No PR triggers the Build action so we'll basically need to test in prod at this point..

Copy link
Contributor

@paulyuk paulyuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@antontroshin
Copy link
Contributor Author

TBH a little bit concerned about the "Validate Go quickstarts on ubuntu-latest" failure https://github.com/dapr/quickstarts/actions/runs/10079513721/job/27868435196

@paulyuk
Copy link
Contributor

paulyuk commented Jul 24, 2024

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.

@paulyuk paulyuk merged commit 407cebd into dapr:master Jul 24, 2024
4 of 7 checks passed
@antontroshin antontroshin deleted the fix-go-tutorial-docker-build branch July 24, 2024 16:20
yaron2 pushed a commit that referenced this pull request Jul 24, 2024
…tClient) (#1062)

* Updating gitignore to exclude out/ from dotnet publish

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Fixing build.yaml github action workflow with modern dockerfiles.  go for dist calculator already fixed by #1058

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

* Updating csharp dockerfile with correct entry point, and do not explicitly expose port

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>

---------

Signed-off-by: Paul Yuknewicz <paulyuk@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Development

Successfully merging this pull request may close these issues.

3 participants