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

feat: Don't require the use of docker group #328

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jpts
Copy link
Contributor

@jpts jpts commented Feb 23, 2022

  • Please check if the PR fulfills these requirements
  • The commit message follows the conventional commits guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
    feat: Don't require the use of docker group, all local invocations of docker are prepended with sudo.

  • What is the current behavior? (You can also link to an open issue here)
    Current scripts assume the use of docker group, which may not be configured for all users

  • What is the new behavior (if this is a feature change)?
    Default to using docker through sudo command, which should work for all users. Docker group users can revert to using just sudo-less docker more easily in the Makefile.

@sublimino
Copy link
Member

Appreciate this is slightly more secure, but might be a breaking change. Can we retain backwards-compatibility?

e.g. test for the existence of the Docker group and use it if configured, automated tests probably won't have sudo access, or will use sudoers which makes this moot in those circumstances.

Alternatively rely on user's aliases?

@jpts
Copy link
Contributor Author

jpts commented Feb 24, 2022

The original motivation for this PR was to prevent breakage for users that don't have the docker group configured. Currently, they have to go through and manually make the above changes to get things working, or re-add their user to the docker group.

Adding the group check and falling back to sudo would be probably the better user experience. Although, we'd have to implement the check in multiple places (Makefile and validate-reqs script), but not the worst thing in the world.

I'm intrigued by your automated testing use-case. Is there a use-case where the user that runs make run won't be root or an admin user with sudo elevation rights, but will be a member of docker group ?

@sublimino
Copy link
Member

The original motivation for this PR was to prevent breakage for users that don't have the docker group configured. Currently, they have to go through and manually make the above changes to get things working, or re-add their user to the docker group.

As it's a change to the status-quo, we can assume it's a breaking change for users in the current configuration. My mindset is to never break things where possible.

I'm intrigued by your automated testing use-case. Is there a use-case where the user that runs make run won't be root or an admin user with sudo elevation rights, but will be a member of docker group ?

I'd assume anything/body currently consuming this either has an alias in place for sudo docker or relies upon the existing mechanism. I'm being kind of speculative without polling existing users, so I try to err on the side of backwards compatibility.

Copy link
Contributor

@denhamparry denhamparry left a comment

Choose a reason for hiding this comment

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

LGTM 🇨🇻

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.

3 participants