-
Notifications
You must be signed in to change notification settings - Fork 825
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
Distroless base image for crd-client #3277
Distroless base image for crd-client #3277
Conversation
Build Succeeded 👏 Build Id: d2c26147-12ff-4132-b51b-850dcdc99e75 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
examples/crd-client/Makefile
Outdated
@@ -25,7 +25,7 @@ | |||
|
|||
REPOSITORY ?= us-docker.pkg.dev/agones-images/examples | |||
|
|||
server_tag = $(REPOSITORY)/crd-client:0.9 | |||
server_tag = $(REPOSITORY)/crd-client:1.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markmandel - how do you feel about going from 0.9 to 1.0 vs. 0.9 to 0.10? Are we happy calling this image a 1.0 image?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point - I figure let's go to 0.10 -- examples can always change I guess (or at least we should be consistent with our examples).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed it to 0.10
examples/crd-client/Dockerfile
Outdated
@@ -14,21 +14,18 @@ | |||
|
|||
# Gather dependencies and build the executable | |||
FROM golang:1.20.4 as builder | |||
WORKDIR /go/src/crd-client | |||
WORKDIR / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why you changed the paths in the build container. That shouldn't affect the location in the final container.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark suggested moving the WORKDIR
path to the root directory ('/').
Example PR: https://github.com/googleforgames/agones/pull/3270/files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was being consistent with everything else I've seen with distroless / done with distroless.
There's no $HOME folders in distroless, so it seems like everyone drops everything in /.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh sorry, I should have looked at this properly - this is the build step.
🤷🏻 actually for this, I have no strong opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on testing, the change to the root directory ('/') for WORKDIR in the build container didn't work as expected, so I've reverted it back.
examples/crd-client/Dockerfile
Outdated
|
||
COPY --from=builder /go/src/crd-client \ | ||
/home/client | ||
COPY --from=builder /client / |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do like just copying the binary over; that seems like an improvement from the previous commands. 👍
For testing for this -- take as standard release/dev Agones install, create this as an image and apply the yaml - it's shouldn't fail, and you can see the output in it's logs. You can see a step through here: |
Build Succeeded 👏 Build Id: 0ecbab4f-836c-4021-8833-4db0282a1451 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
…directory path adjustment
Build Succeeded 👏 Build Id: 64a68baa-45ad-4e73-8b88-0dab2e69fd61 The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Kalaiselvi84, markmandel 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 |
New changes are detected. LGTM label has been removed. |
Build Succeeded 👏 Build Id: 369fdf28-2264-4835-9a55-c36658aa7d3d The following development artifacts have been built, and will exist for the next 30 days:
A preview of the website (the last 30 builds are retained): To install this version:
|
What type of PR is this?
/kind cleanup
What this PR does / Why we need it:
Which issue(s) this PR fixes:
Closes #909
Special notes for your reviewer: