-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Use warnings provided by daemon #1225
Use warnings provided by daemon #1225
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1225 +/- ##
==========================================
+ Coverage 54.77% 54.77% +<.01%
==========================================
Files 292 292
Lines 19275 19285 +10
==========================================
+ Hits 10557 10563 +6
- Misses 8061 8063 +2
- Partials 657 659 +2 |
if info.OSType == "windows" { | ||
return | ||
} | ||
if !info.MemoryLimit { |
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.
nit: I know it is a deprecated function, but we could do some little factorization here 😇
func printWarningLegacy(stdErr io.Writer, property bool, message string){
if !property{
fmt.Println(stdErr, "WARNING: "+message)
}
}
func printWarningsLegacy(dockerCli command.Cli, info types.Info) {
if info.OSType == "windows" {
return
}
stdErr := dockerCli.Err()
printWarningLegacy(stdErr, info.MemoryLimit, "No memory limit support")
printWarningLegacy(stdErr, info. SwapLimit, "No swap limit support")
...
}
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.
Was considering that at some point (also for the warnings returned by the daemon); at least for the daemon-returned warnings, I decided to print them as-is, that way we could use those warnings for other types of messages (INFO: your coffee is ready!
, ERROR: we ran out of milk!
).
We could "engineer" that further (info.level: info
, info.message: "your coffee is ready!"
), but that felt like taking it too far
Also noticed there's another warning that possibly could be moved; https://github.com/thaJeztah/cli/blob/02f48b838f1c61e251d7f9eb6128f589acc84949/cli/command/system/info.go#L127-L130, and wasn't sure if there would be others I'd find
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.
Right and notice the discreet "tabulation" in this warning message 😄
And by the way it was only a nit, I'm fine keeping it as is!
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.
Yeah, need to get moby/moby#37502 merged first as well 😅
02f48b8
to
50f65a6
Compare
ping @silvin-lubecki @vdemeester this should be ready to go now 👍 |
No idea what those failures are; https://jenkins.dockerproject.org/job/docker/job/cli/job/PR-1225/2/execution/node/27/log/. Should not be related to this PR
|
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 🐯
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Warnings are now generated by the daemon, and returned as part of the /info API response. If warnings are returned by the daemon; use those instead of generating them locally. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
50f65a6
to
3c27ce2
Compare
Rebased; vendor bump now only updates docker/docker |
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
CLI changes for moby/moby#37502
Warnings are now generated by the daemon, and returned as part of the
/info
API response.If warnings are returned by the daemon; use those instead of generating them locally.
Engine bump: moby/moby@1800883...2629fe9
Relevant changes: