-
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
Merged
Merged
Vendor cleanup #2124
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
3be74ff
make vendor: always use latest `vndr`
vrothberg ff95af9
pin k8s.io/* to releases and commits
vrothberg 5763927
add hack/tree_status.sh
vrothberg 1a03a0f
travis: integrate vendor checks into the CI
vrothberg fcfd4c4
vendor: add chanages from `vndr`
vrothberg 36da5c7
vendor containernetworking/plugins
vrothberg c710340
vendor ulule/deepcopier
vrothberg 22c4494
update golang.org/x/sys
vrothberg 5d16736
update libpod to v1.1.2
vrothberg 7b39733
travis: use xenial and install libcdev
vrothberg f8a6082
vendor: pin branches to releases or commits
vrothberg 30a207b
vendor.conf: remove unused dependencies
vrothberg 6c463a0
fix gofmt errors
vrothberg f8af473
remove vendor.conf.tmp
vrothberg File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
The table of contents is too big for display.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.