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

fix: enable --env-file flag behavior in nerdctl compose #3703

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

coderbirju
Copy link
Contributor

While debugging this issue on finch - Unable to use the "--env-file" option on "finch compose up" command #890
I noticed that nerdctl also fails to set the env variables within the container when using the "--env-file" option.

$ sudo nerdctl compose up --env-file .env.test show-env
INFO[0000] Creating network testing_default             
INFO[0000] Ensuring image busybox                       
INFO[0000] Creating container testing-show-env-1        
INFO[0000] Attaching to logs                            
show-env-1 |PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
show-env-1 |HOSTNAME=show-env
show-env-1 |HOME=/root
INFO[0001] Container "testing-show-env-1" exited        
INFO[0001] All the containers have exited               
INFO[0001] Stopping containers (forcibly)               
INFO[0001] Stopping container testing-show-env-1

This behavior is also seen on nerdctl compose run command as well.

Digging around the up_service.go file I noticed that we're not passing the --env-file flag to the run command here
Adding --env-file to the Run argument fixes the issue for both compose up and compose run commands.

Testing done:
Verified that the env variables are available within the container.

sudo .././_output/nerdctl compose up --env-file .env.test show-env 
INFO[0000] Ensuring image busybox                       
INFO[0000] Re-creating container testing-show-env-1     
INFO[0000] Running [/local/home/<user>/<project>/nerdctl/_output/nerdctl run --cidfile=/tmp/compose-3618749869/cid -l=com.docker.compose.project=testing -l=com.docker.compose.service=show-env --env-file=.env.test -d --name=testing-show-env-1 --pull=never --net=testing_default --hostname=show-env --restart=no busybox env] 
INFO[0000] Attaching to logs                            
show-env-1 |PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
show-env-1 |A=1
show-env-1 |B=2
show-env-1 |HOSTNAME=show-env
show-env-1 |HOME=/root
INFO[0000] Container "testing-show-env-1" exited        
INFO[0000] All the containers have exited               
INFO[0000] Stopping containers (forcibly)               
INFO[0000] Stopping container testing-show-env-1  

Copy link
Member

@austinvazquez austinvazquez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you have the hood open on this, can you add --env-file reference to docs/command-reference.md#compose? See https://docs.docker.com/reference/cli/docker/compose/ for comparison.

Minor nit: "fix: Fix ..." is redundant.

I think it would be good to add a test for both up and run. A simple container that outputs an environment variable set via environment file should be enough.

@coderbirju coderbirju changed the title fix: Fix --env-file flag behavior in nerdctl compose fix: enable --env-file flag behavior in nerdctl compose Dec 1, 2024
@AkihiroSuda AkihiroSuda added this to the v2.0.2 (tentative) milestone Dec 2, 2024
@AkihiroSuda
Copy link
Member

Needs rebase

@coderbirju
Copy link
Contributor Author

Updated the doc, running into some issues with testing.
Have a test on my fork,
The docker compose command doesn't seem to pass the env-file to the container either. Container logs

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@AkihiroSuda AkihiroSuda merged commit 0c0737d into containerd:main Dec 5, 2024
30 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants