Skip to content

cli/command: use client.CurrentVersion() - #3902

Closed
thaJeztah wants to merge 1 commit into
docker:masterfrom
thaJeztah:use_CurrentVersion
Closed

cli/command: use client.CurrentVersion()#3902
thaJeztah wants to merge 1 commit into
docker:masterfrom
thaJeztah:use_CurrentVersion

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

Make sure the new CurrentVersion() function, which was added in a7e2c3e (#3885), but it looks like I overlooked these uses of what it was replacing 😅

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

@thaJeztahthaJeztah added status/2-code-review kind/refactor PR's that refactor, or clean-up code labels Dec 5, 2022
@thaJeztahthaJeztah added this to the 23.0.0 milestone Dec 5, 2022
@thaJeztah
thaJeztah marked this pull request as draft December 5, 2022 23:31
silvin-lubecki
silvin-lubecki previously approved these changes Dec 6, 2022

@silvin-lubeckisilvin-lubecki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Thanks! I want to have another look at this; there's many places left where we call this (through Client().ClientVersion()), but in most places the Client() is also needed for other purposes (so it wouldn't make much sense to replace it).

That said, I want to look what makes most sense for various locations;

  • Pass the whole DockerCLI around (which includes the Client() API Client
  • Or if we should exactly do the reverse; only pass the APIClient

The reason the whole DockerCLI is passed around (in MOST cases), is only to get the CLI's .Out() or .Err() for printing messages. Perhaps we should pass something through the context or use another approach.

@thaJeztahthaJeztah modified the milestones: 23.0.0, 23.0.1Feb 2, 2023
@thaJeztahthaJeztah modified the milestones: 23.0.1, v-nextFeb 9, 2023
@thaJeztahthaJeztah removed this from the 24.0.0 milestone Apr 27, 2023
Make sure the new CurrentVersion() function, which was added in
a7e2c3e.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@codecov-commenter

codecov-commenter commented Nov 20, 2023

Copy link
Copy Markdown

Codecov Report

Merging #3902 (c0b37c0) into master (45f62db) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files
@@ Coverage Diff @@## master #3902 +/- ##
=======================================
Coverage 59.72% 59.72% =======================================
Files 287 287 Lines 24832 24832 =======================================
Hits 14831 14831 Misses 9115 9115 Partials 886 886 

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/refactorPR's that refactor, or clean-up codestatus/2-code-review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@thaJeztah@codecov-commenter@silvin-lubecki