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

Use ARG instead of ENV for temporary variables #15

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

travier
Copy link
Member

@travier travier commented Feb 16, 2023

We only need those temporary variables for the container build and for the LABELS. We do not want to set those specific environment variables for the container environment itself.

Using ARG instead of ENV lets us do that.

See: containers/toolbox#188

Signed-off-by: Timothée Ravier tim@siosm.fr

@travier
Copy link
Member Author

travier commented Feb 16, 2023

@debarshiray
Copy link
Member

Thanks for digging into this, @travier

If ARG is indeed enough, then feel free to open a pull request to change the canonical sources of the fedora-toolbox images.

@travier
Copy link
Member Author

travier commented Feb 16, 2023

I've opened https://src.fedoraproject.org/container/fedora-toolbox/pull-request/4.

I'm reasonably confident that this is the right change but I still need someone with more Fedora infra experience to confirm it.

@cverna
Copy link
Collaborator

cverna commented Feb 22, 2023

@travier have you tried a scratch build? IIRC, these are used by atomic-reactor during the build so it would be good to double check that builds are still successful after this change.

@cverna
Copy link
Collaborator

cverna commented Feb 22, 2023

Hum ok, from a quick look at atomic-reactor it in fact using the LABELS (https://github.com/containerbuildsystem/atomic-reactor/blob/master/atomic_reactor/plugins/bump_release.py#L263) and not the ENV variables so I think this should work 👍

@travier
Copy link
Member Author

travier commented Feb 22, 2023

How do I trigger a scratch build? I tried:

$ fedpkg container-build --scratch
Could not execute container_build: There are unpushed changes in your repo

@travier
Copy link
Member Author

travier commented Feb 22, 2023

We only need those temporary variables for the container build and for
the LABELS. We do not want to set those specific environment variables
for the container environment itself.

Using ARG instead of ENV lets us do that.

The ARG directive only support one value per line thus psplit the
definition into multiple ones.

See: containers/toolbox#188
See: https://docs.docker.com/engine/reference/builder/#arg

Signed-off-by: Timothée Ravier <tim@siosm.fr>
@debarshiray
Copy link
Member

I've opened https://src.fedoraproject.org/container/fedora-toolbox/pull-request/4.

I'm reasonably confident that this is the right change but I still need someone with more Fedora infra experience to confirm it.

I pulled in the same change to the upstream Toolbx Git repository:
containers/toolbox#1250

@cverna
Copy link
Collaborator

cverna commented Mar 29, 2023

Ok merging this, thanks for fixing this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants