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

containers: use 'podman' or 'sudo docker' in unit-tests scripts #12286

Merged

Conversation

allisonkarlitskaya
Copy link
Member

Detect if we have podman installed and use that in preference to docker.
If we have only docker, make sure to run it under sudo.

This commit is a little bit ugly, particularly in the 'exec' script: we
can end up running sudo more than once for the normal case where we
detect the ID. I have some weird ideas about how to work around that,
but they're pretty evil.

The sed thing is also pretty nasty.

...

@allisonkarlitskaya allisonkarlitskaya added needswork no-test For doc/workflow changes, or experiments which don't need a full CI run, labels Jul 10, 2019
@martinpitt martinpitt self-assigned this Jul 10, 2019
@marusak
Copy link
Member

marusak commented Jul 23, 2019

Needs to rebase to master since #12367 changed tests names

@martinpitt
Copy link
Member

I rebased this, and reworked it a bit:

  • Drop all internal sudo handling. I really don't like scripts that do sudo behind the user's back, it's just prone to hang (sudo asking for password). The current scripts don't do that either.
  • Adjusted the documentation to point out when (not) to use sudo.
  • Do the podman/docker detection in the build script.

I ran containers/unit-tests/build and containers/unit-tests/start locally, and they both succeed with podman as user.

Detect if we have podman installed and use that in preference to docker.

Update the documentation to point out that docker always requires
running as root. With podman, the test can be run in a root or user
instance.

Closes cockpit-project#12286
@martinpitt martinpitt force-pushed the podman-or-sudo-docker branch from e8b8ca5 to 0124798 Compare July 30, 2019 12:18
@martinpitt martinpitt requested a review from croissanne July 30, 2019 12:56
@martinpitt
Copy link
Member

As I now meddled with this, I don't want to approve myself. @Gundersanne, can you please have a look? Perhaps that's not quite what @allisonkarlitskaya is happy with, but we can always iterate in a follow-up.

Copy link
Contributor

@croissanne croissanne left a comment

Choose a reason for hiding this comment

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

So i guess we asume they'll call sudo ./start in case of docker?

@martinpitt
Copy link
Member

@Gundersanne: Yes, that's what you need to do right now in master (e. g. our semaphore tests also run sudo).

@martinpitt martinpitt merged commit d8aa0d2 into cockpit-project:master Jul 30, 2019
@allisonkarlitskaya allisonkarlitskaya deleted the podman-or-sudo-docker branch October 4, 2019 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-test For doc/workflow changes, or experiments which don't need a full CI run,
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants