-
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
Enhance priv. dev. check #11164
Enhance priv. dev. check #11164
Conversation
2912396
to
3038339
Compare
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cevich, 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 tests are red |
3038339
to
85807bf
Compare
LGTM |
Looks like we have a new flake:
|
dd50646
to
e70ebe9
Compare
(IMHO) This PR is ready for merging if there are no further comments or discovered "woopsie". |
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.
It took me a long time to confirm all the changes, and some of the tests are IMO a little incomplete, but that's beyond the scope of this PR. Mostly LGTM aside from one misspelling.
e70ebe9
to
be234be
Compare
This test has been failing for a long time but nobody noticed because CI doesn't have the device node (nested-VM support was disabled). After having enabled nested VM support, tests fail due to some unknown special-handling of this device. Fix both problems by removing the `skip()` and switching to a more generic device which is only present when `--privileged` is used. Signed-off-by: Chris Evich <[email protected]>
Update test to confirm the negative-case, proving the `--privileged` "option is required" for this character device to be present in a container (including rootless). Signed-off-by: Chris Evich <[email protected]>
The `ls` command is not intended for this purpose and may behave in unexpected ways, leading to false positive or negative results. Update the tests to use the purpose built `test` command instead. Also added several *TODO* comments for possible future testing enhancements. Signed-off-by: Chris Evich <[email protected]>
be234be
to
73a755e
Compare
/lgtm |
Update test to confirm the negative-case, proving the
--privileged
"option is required" case.