-
Notifications
You must be signed in to change notification settings - Fork 8.9k
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
update Dockerfiles to remove the warnings #5035
Conversation
images/orderer/Dockerfile
Outdated
@@ -48,7 +48,7 @@ RUN make orderer GO_TAGS=${GO_TAGS} FABRIC_VER=${FABRIC_VER} | |||
############################################################################### | |||
|
|||
ARG UBUNTU_VER | |||
FROM ubuntu:${UBUNTU_VER} | |||
FROM ubuntu:${UBUNTU_VER:-20.04} |
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.
22.04?
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.
fix
08e2dcb
to
a11dd91
Compare
images/baseos/Dockerfile
Outdated
@@ -19,7 +19,7 @@ | |||
ARG GO_VER | |||
ARG UBUNTU_VER | |||
|
|||
FROM ubuntu:${UBUNTU_VER} as base | |||
FROM ubuntu:${UBUNTU_VER:-22.04} AS base |
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.
One of the reasons we use variables like this is so that we only have to maintain them in one place. Setting the default value everywhere defeats the purpose of that.
Is there a compelling reason to set the default?
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.
Is there a compelling reason to set the default?
This is the kind of warning from docker.
https://docs.docker.com/reference/build-checks/invalid-default-arg-in-from/
1 warning found (use docker --debug to expand):
- InvalidDefaultArgInFrom: Default value for ARG ubuntu:${UBUNTU_VER} results in empty or invalid base image name (line 24)
But maybe that's not a good reason.
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.
The risk from that warning is understood and pretty much nil, so I'm thinking it's acceptable to not fix it.
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.
ok. then do we close pr or keep some of the changes?
Signed-off-by: Fedor Partanskiy <fredprtnsk@gmail.com>
a11dd91
to
e3eb778
Compare
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.
Looks good now, thanks!
No description provided.