-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Replace root-based rootless tests #3091
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
Replace root-based rootless tests #3091
Conversation
|
@mheon @giuseppe as we discussed 😄 Assuming this works, may I also strip out the python exec debugging-horror or do we still need that for some other purpose? |
|
Do we need to run any tests as root? Not my call, but I like the thought of running the tests as both root and rootless. |
|
@TomSweeneyRedHat An interesting thought, thx for sharing. I know the number of tests we need to run as root is larger than 1 😄 But I think you're correct that most can be run rootless, and better still, most can probably be run rootless and remotely. I'm sure there are at least a few medium-large testing efficiencies to be gained by consolidating. OTOH, it's likely costly to convert to that scheme in terms of engineer-time on the needed changes @baude WDYT about ^^^ and relating to the remote/local testing split you're working on? Maybe Tom's idea could simplify that? i.e. skip (as-is) rootless testing and only do remote + rootless. Or does that miss too many tests? |
|
I would like as many tests as possible run in all three modes. Root, Rootless, Remote |
1f03528 to
acd5b68
Compare
|
@rhatdan Backstory: The tests removed by this PR are duplicates of rootless tests we otherwise run as a regular user. The removed tests depend on some convoluted, complicated, hard to debug exec wizardry, to start off as root, but then execute podman as a regular user. In other words, these removed tests predate the current CI setup, where we actually run all/most of the integration tests as a regular user on the system, no root involvement at all. The only exception is the case of |
acd5b68 to
4cad3a6
Compare
|
@cevich Ok understood. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cevich, mheon The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Since CI automation is now executing all tests as a regular user, there is no need for root-based testing to run special rootless tests. Remove them. However, the root-based rootless tests did include one test for exercising the '--rootfs' option which is needed. Add a new general, and more through test to replace it - meaning it will be executed as root and non-root. Signed-off-by: Chris Evich <cevich@redhat.com>
4cad3a6 to
ae64e4e
Compare
|
@mheon okay, tests all green (woot!) |
|
LGTM |
|
I'm going to hold this until #3140 lands to keep things clean until the new version lands |
|
Alright, it's landed, let's get this in |
9f69c4e (part of the f31 pr, containers#3091) semi-broke the kill test, there's now an ugly warning: setup(): removing stray images quay.io/libpod/fedora-minimal:latest 7bb5a60e8a78 The comments also didn't actually explain the problem being addressed, and included a misleading reference to busybox. Here we switch to using fedora-minimal only with podman-remote, clean it up (rmi) when finished, and include an explanation in the comments about why this is needed; making it clear that this workaround can be removed once we get rid of podman-remote. We also reformat back to 80 columns. Signed-off-by: Ed Santiago <santiago@redhat.com>
9f69c4e (part of the f31 pr, containers#3091) semi-broke the kill test, there's now an ugly warning: setup(): removing stray images quay.io/libpod/fedora-minimal:latest 7bb5a60e8a78 The comments also didn't actually explain the problem being addressed, and included a misleading reference to busybox. Here we switch to using fedora-minimal only with podman-remote, clean it up (rmi) when finished, and include an explanation in the comments about why this is needed; making it clear that this workaround can be removed once we get rid of podman-remote. We also reformat back to 80 columns. Signed-off-by: Ed Santiago <santiago@redhat.com>
Since CI automation is now executing all tests as a regular user, there
is no need for root-based testing to run special rootless tests. Remove
them.
However, the root-based rootless tests did include one test for exercising
the '--rootfs' option which is needed. Add a new general, and more through
test to replace it - meaning it will be executed as root and non-root.
Signed-off-by: Chris Evich cevich@redhat.com