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

CLI flag for running container as the current user #3499

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kponichtera
Copy link

@kponichtera kponichtera commented Mar 27, 2022

Closes #3498

- What I did

I’ve added the --current-user boolean flag to the docker run command to avoid docker run -u $(id -u):$(id -g) construct which often appears in the scripts that use Docker containers for development.

- How I did it

The user parameter of the Docker API call is now built by the dedicated parseUser function in opts.go file. It checks if --current-user flag was set. In such case, it fetches current user UID and GID using Go’s standard library, builds the string UID:GID, and passes it to the Docker API. The --user flag has precedence over --current-user - if both are specified, the --user one will be taken.

There is also a unit test, which verifies the behavior of the function that builds the API call user field. I’ve also updated bash and zsh autocompletion files, as well as the Markdown reference document for the run command.

If there are some things that should be done and which I missed, please let me know and I will address them.

- How to verify it

  1. Build repo with docker buildx bake
  2. Run ./build/docker run -it --current-user ubuntu /bin/bash - the Ubuntu container will run the process inside with the same UID and GID as the CLI caller’s process.
  3. Run ./build/docker run -it --current-user -u 1234:1234 ubuntu /bin/bash - the -u flag will take precedence

- Description for the changelog

Added --current-user flag to the docker run command.

- A picture of a cute animal (not mandatory but encouraged)

cute_animal

Signed-off-by: Konrad Ponichtera <konpon96@gmail.com>
@kponichtera kponichtera marked this pull request as ready for review March 27, 2022 01:18
@codecov-commenter
Copy link

codecov-commenter commented Mar 27, 2022

Codecov Report

Merging #3499 (dea9b89) into master (e1d4324) will increase coverage by 0.00%.
The diff coverage is 78.57%.

@@           Coverage Diff           @@
##           master    #3499   +/-   ##
=======================================
  Coverage   59.01%   59.02%           
=======================================
  Files         284      284           
  Lines       23833    23846   +13     
=======================================
+ Hits        14066    14076   +10     
- Misses       8908     8910    +2     
- Partials      859      860    +1     

@kponichtera
Copy link
Author

I have modified the code and implemented the suggestion, mentioned in #3498. Instead of new --current-user flag, running the container as current user is now done though setting already existing --user flag to value current.

@albers
Copy link
Collaborator

albers commented Mar 28, 2022

Please see the recent discussion in #3498 (comment)

@kponichtera
Copy link
Author

@albers Looking at the discussion in #3498 it seems like a dedicated --current-user flag would be a better solution than trying to find a safe value for the --user one. Should I revert this PR to state with the --current-user flag?

@albers
Copy link
Collaborator

albers commented Mar 30, 2022

Should I revert this PR to state with the --current-user flag?

I think, yes.
@thaJeztah @ndeloof WDYT?

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.

Feature request: CLI flag for running container as the current user
6 participants