Skip to content
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

cli/command/container: runStats(): refactor and (linting) fixes #4729

Merged
merged 3 commits into from
Dec 20, 2023

Conversation

thaJeztah
Copy link
Member

cli/command/container: runStats(): minor cleanup and fixes

  • memoize the API-client in a local variable.
  • use struct-literals in some places.
  • rename some variables for clarity and to prevent colliding with imports.
  • make use of the event-constants (events.ContainerEventType).
  • fix some grammar
  • fix some minor linting warnings

cli/command/container: runStats(): don't register unused event handlers

We were unconditionally registering event-handlers for these events, but
the handler itself would ignore the event depending on the "all" option.

This patch skips registering the event handlers, so that we're not handling
them (saving some resources).

cli/command/container: runStats(): move code to where it's used

The monitorContainerEvents and getContainerList closures where only
used when collecting "all" containers, so let's define them in that
branch of the code.

Also move some of the other variables closer to where they're used.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

- memoize the API-client in a local variable.
- use struct-literals in some places.
- rename some variables for clarity and to prevent colliding with imports.
- make use of the event-constants (events.ContainerEventType).
- fix some grammar
- fix some minor linting warnings

Signed-off-by: Sebastiaan van Stijn <[email protected]>
We were unconditionally registering event-handlers for these events, but
the handler itself would ignore the event depending on the "all" option.

This patch skips registering the event handlers, so that we're not handling
them (saving some resources).

Signed-off-by: Sebastiaan van Stijn <[email protected]>
The monitorContainerEvents and getContainerList closures where only
used when collecting "all" containers, so let's define them in that
branch of the code.

Also move some of the other variables closer to where they're used.

Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Dec 20, 2023
@thaJeztah thaJeztah added this to the 25.0.0 milestone Dec 20, 2023
@thaJeztah thaJeztah self-assigned this Dec 20, 2023
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2023

Codecov Report

Merging #4729 (633ba88) into master (1f97a34) will increase coverage by 0.01%.
The diff coverage is 0.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4729      +/-   ##
==========================================
+ Coverage   59.66%   59.68%   +0.01%     
==========================================
  Files         287      287              
  Lines       24760    24755       -5     
==========================================
  Hits        14774    14774              
+ Misses       9100     9095       -5     
  Partials      886      886              

@thaJeztah thaJeztah merged commit 3b57acb into docker:master Dec 20, 2023
76 checks passed
@thaJeztah thaJeztah deleted the stats_cleanup branch December 20, 2023 12:04
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.

3 participants