-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Add verification code to verify user input on resources. #60
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
Conversation
|
@rhatdan looks like ci is failing with a missing dependency on containerd? |
cmd/kpod/create_cli.go
Outdated
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.
would it make sense to declare a string, append it to warnings then send it to logrus.Warn to avoid the duplications?
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.
Makes sense.
cmd/kpod/create_cli.go
Outdated
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.
If you have multiple warnings packed into 'warnings' will it be readable? Can we stuff a \n at the end of each?
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.
warnings is a list of strings, I believe. The caller would be responsible for how he wants to display the warnings.
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.
Added an addWarnings function
cmd/kpod/spec.go
Outdated
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.
would "<0" be better here? I think it's pretty controlled now, but might be a good hedge long-term.
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.
-1 seems to be the special value (Default which indicates whether to use the Swappiness or not. (At least in Docker.)
cmd/kpod/spec.go
Outdated
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 it's from #54 except it seems to be missing the latest changes from there
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.
That's why this is a WIP, waiting for it to get merged, then I can rebase and remove WIP
|
☔ The latest upstream changes (presumably 91b406e) made this pull request unmergeable. Please resolve the merge conflicts. |
Update version of docker to pull in lates code Remove kubernetes since libpod is not tied to it. Remove a few other packages that we don't seem to use. Left in the networking stuff, since we will hopefully be wiring that together. Signed-off-by: Daniel J Walsh <[email protected]>
|
Changes LGTM, tests are not at all happy. |
Added lots of verification code to make sure resourses asociated with containers is correct. Signed-off-by: Daniel J Walsh <[email protected]>
|
@TomSweeneyRedHat Tests are passing now. |
|
LGTM |
|
@rh-atomic-bot r=umohnani8 |
|
📌 Commit 616667b has been approved by |
|
⌛ Testing commit 616667b with merge 195d48d... |
Added lots of verification code to make sure resourses asociated with containers is correct. Signed-off-by: Daniel J Walsh <[email protected]> Closes: #60 Approved by: umohnani8
|
☀️ Test successful - status-papr |
This directory just had Markdown and vendor.conf. I'm not sure why we have it in our version control, maybe old versions of vndr kept it? Or maybe folk dropped it into vendor/ by hand without using vndr? The history of that vendored directory is: * 619637a (Handle Linux Capabilities from command line, 2017-11-03, containers#17) added the three files to our version control. * c344fe6 (Update vendoring, 2017-11-22, containers#60) bumped hack/README.md. * af64e10 (Vendor in lots of kubernetes stuff to shrink image size, 2018-03-26, containers#554) bumped hack/README.md. * 27107fd (Vendor in latest containers/image and contaners/storage, 2018-04-18, containers#509) removed the files. * a824186 (Use buildah commit and bud in podman, 2018-04-25, containers#681) added the files back. * I'm removing them again in this commit. With this commit, $ vndr github.com/docker/docker becomes a no-op. Signed-off-by: W. Trevor King <[email protected]>
This directory just had Markdown and vendor.conf. I'm not sure why we have it in our version control, maybe old versions of vndr kept it? Or maybe folk dropped it into vendor/ by hand without using vndr? The history of that vendored directory is: * 619637a (Handle Linux Capabilities from command line, 2017-11-03, #17) added the three files to our version control. * c344fe6 (Update vendoring, 2017-11-22, #60) bumped hack/README.md. * af64e10 (Vendor in lots of kubernetes stuff to shrink image size, 2018-03-26, #554) bumped hack/README.md. * 27107fd (Vendor in latest containers/image and contaners/storage, 2018-04-18, #509) removed the files. * a824186 (Use buildah commit and bud in podman, 2018-04-25, #681) added the files back. * I'm removing them again in this commit. With this commit, $ vndr github.com/docker/docker becomes a no-op. Signed-off-by: W. Trevor King <[email protected]> Closes: #752 Approved by: baude
No description provided.