Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jun 18, 2024

Some leftovers from work on other PRs

daemon/cluster: minor linting issues and cleanup

  • rename variables that shadowed imports
  • remove some intermediate vars
  • slight reformating for readability

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)

- rename variables that shadowed imports
- remove some intermediate vars
- slight reformating for readability

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Jun 18, 2024
@thaJeztah thaJeztah self-assigned this Jun 18, 2024
Comment on lines -61 to +62
}); err != nil {
})
if err != nil {
Copy link
Member Author

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

@thaJeztah thaJeztah changed the title daemon, daemon/cluster: minor linting issues and cleanups daemon, daemon/cluster, integration/container: minor linting issues and cleanups Jun 18, 2024
@thaJeztah thaJeztah added this to the 27.0.0 milestone Jun 18, 2024
Comment on lines 249 to 250
if !containertypes.NetworkMode(sn.Type()).IsPrivate() ||
!containertypes.NetworkMode(n.Type()).IsPrivate() {
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Copy link
Member Author

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>
@thaJeztah
Copy link
Member Author

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

@thaJeztah thaJeztah merged commit c321730 into moby:master Jun 18, 2024
@thaJeztah thaJeztah deleted the leftover_nits branch June 18, 2024 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactor PR's that refactor, or clean-up code status/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants