-
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
APIv2 tests: followup to recent log test #12925
Conversation
Followup to containers#12919, which merged while I was writing review feedback. This actually confirms log output. This required a minor change to the 't' helper: stripping NUL chars from the http result. And, while I'm at it, a bunch of cleanup for running rootless: - set $CONTAINERS_HELPER_BINARY_DIR, so we can find rootlessport - add a few conditionals for different expectations Signed-off-by: Ed Santiago <[email protected]>
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago 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 |
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
Great work, @edsantiago, thank you!
(I realize that this test does not run rootless in CI, nor is that a priority. I believe it may one day be useful to do so, and these changes are pretty small, and don't really hurt readability. This may help us one later day) |
/hold |
networkmode=slirp4netns | ||
if root; then | ||
networkmode=bridge | ||
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.
This looks like a bug we should also default to bridge as rootless for the compat API, @mheon WDYT?
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.
@Luap99 thank you for this. Addressing that is beyond the scope of this PR (and beyond my ability), so I've released the hold. If this situation needs fixing, it should be done in a followup PR.
Followup to #12919, which merged while I was writing
review feedback. This actually confirms log output.
This required a minor change to the 't' helper: stripping
NUL chars from the http result.
And, while I'm at it, a bunch of cleanup for running rootless:
Signed-off-by: Ed Santiago [email protected]