-
Notifications
You must be signed in to change notification settings - Fork 792
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
Test namespace flags of 'bud' subcommand #3173
Test namespace flags of 'bud' subcommand #3173
Conversation
Add 'bud' subcommand to the 'namespaces' system test, which already verifies namespace flags with 'from' and 'run' subcommands. Signed-off-by: Hironori Shiina <[email protected]>
LGTM |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hshiina, rhatdan 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 but I'm not an expert in these test. @edsantiago will this new test need any special treatment for reuse in the 'bud' testing for podman? |
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 at first pass, but this requires more than one pass: it's pretty complicated. ITM I have one request. It may turn out to be impossible or too difficult to refactor, that's ok, but I'd like to know that at least it has been considered.
|
||
|
||
# Also test bud command |
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.
The idmapping
test was unmaintainable even before this addition; it is now even more so. I like the addition, and it is well done, but I wonder if you could take some time to find a way to refactor this, perhaps by crafting one or more smaller helper functions?
@cevich this will never run under podman. Podman only tests |
Introduce helper functions for the same check between 'from and run' and 'bud'. Signed-off-by: Hironori Shiina <[email protected]>
Oh, nice, thank you! It'll take me a while to review in depth, but I fully intend to do so this morning! |
/lgtm Nice work! |
Followup to containers#3173 - just a little further cleanup of idmapping test. Plus, oops, fix a longstanding error-failure bug Signed-off-by: Ed Santiago <[email protected]>
Signed-off-by: Hironori Shiina [email protected]
What type of PR is this?
/kind other
What this PR does / why we need it:
Add 'bud' subcommand to the 'namespaces' system test, which already verifies namespace flags with 'from' and 'run' subcommands.
How to verify it
Run the system test:
bats tests/namespaces.bats
Which issue(s) this PR fixes:
None
Special notes for your reviewer:
Does this PR introduce a user-facing change?