Skip to content

test/system: two small fixes #27233

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 6, 2025
Merged

test/system: two small fixes #27233

merged 2 commits into from
Oct 6, 2025

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Oct 6, 2025

Follow up to #27230

Does this PR introduce a user-facing change?

None

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 6, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2025
@Luap99 Luap99 mentioned this pull request Oct 6, 2025
Merged
@Luap99
Copy link
Member Author

Luap99 commented Oct 6, 2025

cc @ricardobranco777 @timcoding1988 @baude @Honny1

ricardobranco777
ricardobranco777 suggested changes Oct 6, 2025
View reviewed changes
Honny1
Honny1 reviewed Oct 6, 2025
View reviewed changes
Copy link
Member

@Honny1 Honny1 left a comment

Choose a reason for hiding this comment

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

I have one question about a possibly missed quote; otherwise, LGTM.

The CI is failing due to what looks like an unrelated linter issue. I think rebasing this PR on the latest main might solve it; otherwise, I'm not sure how to proceed.

@Luap99
Copy link
Member Author

Luap99 commented Oct 6, 2025

The CI is failing due to what looks like an unrelated linter issue. I think rebasing this PR on the latest main might solve it; otherwise, I'm not sure how to proceed.

I am fully rebased, it is main that got broken due parallel merge of indirectly conflicting PRs #27126 (unused linter) and #27169 (quadlet template fix). So someone must submit a PR with the linter fix that must get merged first then all other PRs must rebase to get unblocked.

@Honny1
Copy link
Member

Honny1 commented Oct 6, 2025

I created a PR with a fix for the linter: #27234

Luap99 added 2 commits October 6, 2025 16:01
@Luap99
test/system: run_podman ? needs quoting
fdb5ac5 
Bash will expand a signle ? to a file name which consists of a single
char, and thus if you have a file named "a" in the cwd it will add a as
argument which causes podman a ... to be executed which clearly fails
the test.

Signed-off-by: Paul Holzinger <[email protected]>
@Luap99
test/system: actually wait for container removal
3e774ee 
podman wait by default waits for exit not removal as the man page
documents.

Fixes: 3a98b6d ("test: Wait for killed container to avoid leak")

Signed-off-by: Paul Holzinger <[email protected]>
@baude
Copy link
Member

baude commented Oct 6, 2025

LGTM

@ricardobranco777
Copy link
Contributor

Thanks for fixing this!

@ricardobranco777
Copy link
Contributor

Verification run for 130-kill passed in the rootless/remote case that motivated the previous PR:

https://openqa.opensuse.org/tests/5366766

@ricardobranco777 ricardobranco777 mentioned this pull request Oct 6, 2025
Merged
@Honny1
Copy link
Member

Honny1 commented Oct 6, 2025

LGTM, CI is happy

@Honny1
Copy link
Member

Honny1 commented Oct 6, 2025

/LGTM

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit 14b68ba into containers:main Oct 6, 2025
55 checks passed
@Luap99 Luap99 deleted the test-fix branch October 6, 2025 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@Honny1 Honny1 Honny1 left review comments

@ricardobranco777 ricardobranco777 ricardobranco777 requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@Honny1 Honny1

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. release-note-none

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Luap99 @Honny1 @baude @ricardobranco777