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

Align Dockerfile builder image #39

Merged
merged 1 commit into from
Jan 16, 2025
Merged

Align Dockerfile builder image #39

merged 1 commit into from
Jan 16, 2025

Conversation

jmhbnz
Copy link
Member

@jmhbnz jmhbnz commented Jan 15, 2025

Ensure golang patch version is specified.
Base of Debian Bookworm in line with other etcd projects.

Addresses: #37 (comment)

cc @ahrtr

Ensure golang patch version is specified.
Base of Debian Bookworm in line with other etcd projects.

Signed-off-by: James Blair <[email protected]>
@jmhbnz
Copy link
Member Author

jmhbnz commented Jan 15, 2025

Test failure addressed by kubernetes/test-infra#34146

@jmhbnz
Copy link
Member Author

jmhbnz commented Jan 15, 2025

/retest

@ivanvc
Copy link
Member

ivanvc commented Jan 15, 2025

Follow up on test failure at PR: kubernetes/test-infra#34147

@ivanvc
Copy link
Member

ivanvc commented Jan 15, 2025

/retest

@ahrtr
Copy link
Member

ahrtr commented Jan 15, 2025

/test pull-etcd-operator-test-e2e

@hakman
Copy link
Contributor

hakman commented Jan 15, 2025

LGTM

@hakman
Copy link
Contributor

hakman commented Jan 15, 2025

@jmhbnz I am curious, why do we set toolchain version in the go.mod file?

@ahrtr
Copy link
Member

ahrtr commented Jan 15, 2025

@jmhbnz I am curious, why do we set toolchain version in the go.mod file?

It should be a best practice. Pinning to a specific/consistent go version ensures we have the same go version in test and production environment, also ensure we always build the binary/image with a safe (usually the latest patch version) go version.

@hakman
Copy link
Contributor

hakman commented Jan 15, 2025

@jmhbnz I am curious, why do we set toolchain version in the go.mod file?

It should be a best practice. Pinning to a specific/consistent go version ensures we have the same go version in test and production environment, also ensure we always build the binary/image with a safe (usually the latest patch version) go version.

I was a little confused on what Go version GHA uses from the go.mod file, either toolchain go1.23.4 or go 1.23.0.
Maybe we need to set the Go version to the toolchain version.

2025-01-15T09:52:28.3490983Z Adding to the cache ...
2025-01-15T09:52:32.5799032Z Successfully cached go to /opt/hostedtoolcache/go/1.23.0/x64
2025-01-15T09:52:32.5800450Z Added go to the path
2025-01-15T09:52:32.5803586Z Successfully set up Go version 1.23.0
2025-01-15T09:52:36.6251957Z go: downloading go1.23.4 (linux/amd64)
2025-01-15T09:52:36.6408632Z [command]/opt/hostedtoolcache/go/1.23.0/x64/bin/go env GOMODCACHE
2025-01-15T09:52:36.6447123Z [command]/opt/hostedtoolcache/go/1.23.0/x64/bin/go env GOCACHE

@ahrtr
Copy link
Member

ahrtr commented Jan 15, 2025

I was a little confused on what Go version GHA uses from the go.mod file, either toolchain go1.23.4 or go 1.23.0.
Maybe we need to set the Go version to the toolchain version.

Good catch, we should follow the toolchain, just raised #41

@ahrtr
Copy link
Member

ahrtr commented Jan 15, 2025

Also I am thinking probably setting a tag 1.23.4-bookworm should be enough. There is no need to add a digest (SHA 256 value). I assume that the tag and digest is 1:1 mapping.

@hakman
Copy link
Contributor

hakman commented Jan 15, 2025

Also I am thinking probably setting a tag 1.23.4-bookworm should be enough. There is no need to add a digest (SHA 256 value). I assume that the tag and digest is 1:1 mapping.

It is not enough to use the tag, they regularly rebuild with newer bookworm base image.

Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

Thanks @jmhbnz

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, jmhbnz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ivanvc
Copy link
Member

ivanvc commented Jan 15, 2025

/retest

@jmhbnz jmhbnz merged commit 6662396 into etcd-io:main Jan 16, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants