-
-
Notifications
You must be signed in to change notification settings - Fork 44
fix: fix some warnings when building images #295
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
Conversation
kerthcet
left a comment
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.
Thanks @nayihz left some comments, not mandatory but part of the project style I think.
| @@ -1,10 +1,10 @@ | |||
| ARG BASE_IMAGE | |||
| ARG BUILDER_IMAGE | |||
| ARG BASE_IMAGE=gcr.io/distroless/static:nonroot | |||
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.
We use makefile in our project, use Dockerfile directly is not a recommended way, and we don't want to spread the configurations across the project, for example, once the golang version changes, we only need to update the Makefile, which makes more sense to me.
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.
So let's not pass the args to the Dockerfile at all. Default it in the Makefile.
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.
According to the official documentation, it is recommended to set a default value in Dockerfile. Otherwise there will be warning messages.
https://docs.docker.com/reference/build-checks/invalid-default-arg-in-from/#examples
once the golang version changes, we only need to update the Makefile
This is just the default values which only take effect when these parameter is not provided. I think we still could only need to update Makefile in this scenario.
|
/lgtm Thanks @nayihz |
|
/kind cleanup |
What this PR does / why we need it
Which issue(s) this PR fixes
Fixes #
There are some warnings when build image.
Special notes for your reviewer
Does this PR introduce a user-facing change?