-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
system tests: safer install_kube_template() #24515
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically that would cause a regression for distros not installing podman to /usr/bin/podman
and running these tests outside of our source dir as they are no longer covered. I doubt that will be an issue and we are skipping not failing so it seems good enough to me
LGTM (I am fine without the elif comment addressed)
test/system/helpers.systemd.bash
Outdated
else | ||
if [[ "$PODMAN" != "/usr/bin/podman" ]]; then | ||
skip "No $unit_file_in, and PODMAN=$PODMAN" | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this could use an elif
to avoid one level of nesting.
Previous version was badly broken: it relied on 'make' rebuilding a file under cwd, which is a no-no; and, in the case where we don't have a source directory, just blindly hoped that there'd be a system-installed .service file with the correct path to podman. Solution: . if running in source directory, run sed directly into destination service file in $UNIT_DIR. This is ugly duplication of a line in Makefile. . if NOT running in a source directory, check $PODMAN: . if it's /usr/bin/podman, continue. Include a warning that will be shown only on test failure. . otherwise skip, because we don't know what we're testing Signed-off-by: Ed Santiago <[email protected]>
5560470
to
9694177
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, Luap99 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/lgtm |
(Import from #23275, where it has been working for weeks. More importantly, I tested it on 1mt)
Previous version was badly broken: it relied on 'make' rebuilding a file under cwd, which is a no-no; and, in the case where we don't have a source directory, just blindly hoped that there'd be a system-installed .service file with the correct path to podman.
Solution:
. if running in source directory, run sed directly into
destination service file in $UNIT_DIR. This is ugly
duplication of a line in Makefile.
. if NOT running in a source directory, check $PODMAN:
. if it's /usr/bin/podman, continue
. otherwise skip, because we don't know what we're testing