Various cleanups #587
Various cleanups #587
Conversation
Instead, let's call podman-remote with an explicit `--url`. Co-Authored-By: Martin Pitt <mpitt@redhat.com>
This avoids label mismatches. It doesn't matter for `local-files/`, we could fix them with adding the missing `:z` options. But we don't want to relabel the podman socket in the running system.
Introduced in cockpit-project/bots@85b0db4a0bfd5 This was the last volume mount into the home directory, so clean up the directory creation. Also drop the long-obsolete ~/.rhel directory.
mock-github-pr gets copied in at runtime in `run-local.sh`, and the OpenShift resources don't end up in the container either.
This causes a "will soon be removed" warning, see https://github.blog/changelog/2022-10-11-github-actions-deprecating-save-state-and-set-output-commands/
Always start with the dummy token in the initialization, as most tests don't need it at all, or even expect to run against the local container setup. This is particularly relevant for `test_mock_pr()`. There are only two places where we want the actual token: `test_pr()` and `--interactive` mode. Both of these already opt into the real token by updating `.config--github-token`.
| @@ -68,7 +68,6 @@ ExecStart=/usr/bin/podman run --name=cockpit-tasks-%i --hostname=${CONTAINER_HOS | |||
| --volume=%t/podman/podman.sock:/podman.sock:rw \ | |||
| --env=COCKPIT_GITHUB_TOKEN_FILE=/run/secrets/webhook/.config--github-token \ | |||
| --env=COCKPIT_S3_KEY_DIR=/run/secrets/tasks/s3-keys \ | |||
| --env=CONTAINER_HOST=unix:///podman.sock \ | |||
There was a problem hiding this comment.
I don't think we ever resolved what we want to do about this path, but I actually don't care. It's not really API.
tasks/run-local.sh
Outdated
| @@ -177,8 +181,9 @@ EOF | |||
| # Run tasks container in the backgroud | |||
tasks/Containerfile
Outdated
| @@ -78,6 +78,7 @@ RUN groupadd -g 1111 -r user && useradd -r -g user -u 1111 user --home-dir /work | |||
| echo 'user ALL=NOPASSWD: /usr/bin/chmod 666 /dev/kvm' > /etc/sudoers.d/user-fix-kvm | |||
|
|
|||
| ENV LANG=C.UTF-8 \ | |||
| HOME=/work \ | |||
There was a problem hiding this comment.
That feels wrong. This is going to have weird effects when we try to enter the container as -u root, for example. Why doesn't this get set normally?
Checking this locally (against the current image, without this change):
$ podman run --rm quay.io/cockpit/tasks:latest sh -c 'echo $HOME'
/work
There was a problem hiding this comment.
Also: it occurs to me that now that we no longer have any "homedir path API" bindmounts, we could probably get rid of a great deal of the above special handling of creating /work and everything inside of it and let the homedir land in the default location.
Just about the only thing that looks actually useful is the gitconfig, and I have to think we could do something about that by setting some environment variables when we're running the bots... (or just set the environment variables on the container in the usual way when creating it)...
There was a problem hiding this comment.
It gets set "normally" though user's home dir, but that doesn't exist. But also, not super important, I'll just back out that commit.
| @@ -174,7 +174,7 @@ EOF | |||
| sleep 3 | |||
| done | |||
|
|
|||
| # Run tasks container in the backgroud | |||
| # Run tasks container in the background | |||
- In `test_mock_pr()` we expect the inner test to actually succeed, not just finish. Same as in `test_pr()`. - In `test_mock_pr()`, add the "Running on <hostname>" assertion that we already have in `test_pr()`. This makes it more symmetric and also easier to validate the fix in the job-runner PR. - `job-runner` has a different success message, but doesn't show the test exit code. Add this as alternative accepted pattern.
This is a grab-bag of small unrelated stuff. @allisonkarlitskaya I took in and fixed your
$CONTAINER_HOSTchange from #586. Can you please testtasks/run-local.shwith this? With any luck it fixes it. If not, I'm also happy to add--security-opt=label=disableinstead like in commit 84e8270