-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Vendor cleanup #2124
Vendor cleanup #2124
Changes from all commits
3be74ff
ff95af9
5763927
1a03a0f
fcfd4c4
36da5c7
c710340
22c4494
5d16736
7b39733
f8a6082
30a207b
6c463a0
f8af473
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
#!/bin/bash | ||
set -e | ||
|
||
STATUS=$(git status --porcelain) | ||
if [[ -z $STATUS ]] | ||
then | ||
echo "tree is clean" | ||
else | ||
echo "tree is dirty, please commit all changes and sync the vendor.conf" | ||
echo "" | ||
echo "$STATUS" | ||
exit 1 | ||
fi |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,12 +9,16 @@ import ( | |
) | ||
|
||
func (c *ContainerServer) addSandboxPlatform(sb *sandbox.Sandbox) { | ||
c.state.processLevels[selinux.NewContext(sb.ProcessLabel())["level"]]++ | ||
// NewContext() always returns nil, so we can safely ignore the error | ||
ctx, _ := selinux.NewContext(sb.ProcessLabel()) | ||
c.state.processLevels[ctx["level"]]++ | ||
} | ||
|
||
func (c *ContainerServer) removeSandboxPlatform(sb *sandbox.Sandbox) { | ||
processLabel := sb.ProcessLabel() | ||
level := selinux.NewContext(processLabel)["level"] | ||
// NewContext() always returns nil, so we can safely ignore the error | ||
ctx, _ := selinux.NewContext(processLabel) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The same for here. Another option would be to revert the selinux-go related changes and put it in a separate PR like this: #2127 WDYT? (If you disagree I will close my PR) |
||
level := ctx["level"] | ||
pl, ok := c.state.processLevels[level] | ||
if ok { | ||
c.state.processLevels[level] = pl - 1 | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 would still go for the error handling here since I have the feeling that it will be missed when the day comes that
selinux.NewContext
returns a real error.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.
Agreed, from a maintainability perspective we should catch if there ever becomes an error
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 think that @rhatdan wants to get rid of those entirely by letting containers/storage handle all label issue but I can update the code now to be on the safe side.
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've had a look at the call stack and don't think it's worth the effort. We had to change a whole bunch of APIs up to the ContainerServer which does not really implement error handling (even for {Add,Remove}Container).
I agree that errors shouldn't be ignored but given this affects large parts of the API, I prefer to defer this to a separate PR.