-
Notifications
You must be signed in to change notification settings - Fork 18.9k
daemon, daemon/cluster, integration/container: minor linting issues and cleanups #48022
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
- rename variables that shadowed imports - remove some intermediate vars - slight reformating for readability Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
| }); err != nil { | ||
| }) | ||
| if err != nil { |
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.
Mostly to align with other code in this file, and slightly more readable for these long blocks
| if !containertypes.NetworkMode(sn.Type()).IsPrivate() || | ||
| !containertypes.NetworkMode(n.Type()).IsPrivate() { |
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.
Is this causing the linter to complain? To be honest I find the previous one to be more readable.
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 one not, but it made my brain complain as I initially thought there was some nested conditions here, and having them on a single line made that more clear to me.
Do you want me to change it back? I can drop this commit if you want to.
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 makes it much more visible that it's the same check, just on the different variable sn vs n.
I would be more happy to drop it, but it's not a blocker also, so I'll leave it up to you.
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 dropped the commit 👍
The canonical alias is "containertypes" for this import. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The canonical alias is "containertypes" for this import. Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
|
I dropped that commit; I'll bring this one in once the "bin-image" steps are green; no need to re-run all of CI |
Some leftovers from work on other PRs
daemon/cluster: minor linting issues and cleanup
daemon: updateNetworkSettings: unwrap lines for readability
daemon/cluster/executor/container: use consistent alias for import
The canonical alias is "containertypes" for this import.
integration/container: use consistent alias for import
The canonical alias is "containertypes" for this import.
- A picture of a cute animal (not mandatory but encouraged)