Closed ydirson closed 7 months ago
Nicely done!
Can you leave out 8a87c7c please
using
ARG
was done on purpose, see #593 and for example https://github.com/cross-rs/cross/wiki/Recipes#dockerfile where we recommend setting it, I do agree with changing it though, making it propagate into the images probably makes sense
I see this comes from the exact same reason I'm proposing this change.
The problem I see is that ARG
is supposed to set a var that's local to the recipe, and no dockerfile reads it, so as it stands it seems to have no effect (according to the spec). So maybe docker and podman interpret the thing differently, which would likely be a bug in one or more of docker|podman|docker's doc.
We can add an ENV
statement in addition to ARG
, but I don't see the point if all we want is to add this var to the env, that's what ENV
is for.
My original review message got removed so I just quickly rewrote it, what I meant to add to my sentiment about 8a87c7c is that it should be a separate pr!
ARG
is in dockerfiles seen by RUN
My original review message got removed so I just quickly rewrote it, what I meant to add to my sentiment about 8a87c7c is that it should be a separate pr!
:+1:
ARG
is in dockerfiles seen byRUN
This says the variables are available in the RUN
command itself, not that they are added to the RUN
commands' environment. Especially this statement and example make it clear:
Using the example above but a different ENV specification you can create more useful interactions between ARG and ENV instructions:
FROM ubuntu ARG CONT_IMG_VER ENV CONT_IMG_VER=${CONT_IMG_VER:-v1.0.0} RUN echo $CONT_IMG_VER
ENV CONT_IMG_VER=${CONT_IMG_VER:-v1.0.0}
would not have any usefulness above ENV CONT_IMG_VER=v1.0.0
.
To me:
ARG
is clearly to allow customization of the build using --build-arg
, and I don't see that we need it for DEBIAN_FRONTEND
ENV
is clearly to set an env variable, and that one we do needARG
vars are also promoted to ENV
?I wouldn't say it's unwanted, I've seen it used in other images for non-persistant environment variables. Anyway, I think we can drop the subject about ARG
/ENV
and just make the change in another pr :D
Is this ready?
/ci try
edit: ugh, why does gha do this.....
Is this ready?
I'd think so
/ci try
Starting try run. Link to action
Rerolling #1401 following GH closing it on push mistake - :sigh: