Skip to content

cli-plugins/manager: minor cleanups and refactoring#5936

Merged
thaJeztah merged 5 commits intodocker:masterfrom
thaJeztah:plugin_manager_cleanups
Mar 19, 2025
Merged

cli-plugins/manager: minor cleanups and refactoring#5936
thaJeztah merged 5 commits intodocker:masterfrom
thaJeztah:plugin_manager_cleanups

Conversation

@thaJeztah
Copy link
Copy Markdown
Member

cli-plugins/manager: ListPlugins: pass context to error-group

This error-group was added in 89583b9, but
passed a context.TODO because the function didn't have a context as argument.

However, it does get the root-command passed, which holds the context, so
we can pass that.

cli-plugins/manager: rename var that shadowed arg in test

cli-plugins/manager: add test for empty / non-existing plugin dirs

Verify that listPluginCandidates returns an empty result if nothing was
found.

cli-plugins/manager: ListPlugins: return early if no candidates

Skip the other logic, which includes listing all commands provided; if
there's no plugin-candidates, those steps won't be needed.

cli-plugins/manager: getPluginDirs: remove redundant error-return

This function returned an error (if any) from config.Path. However, the
only situation in which an error could be returned was if the given path
to append to config.Dir was outside of the config directory. This can
only happen if the path to append would try to traverse directories (e.g.,
passing ../../cli-plugins).

Given that we're passing a hard-coded value, that would not be the case,
so we can simplify the code to join the path directly, and don't have to
handle errors.

- Human readable description for the release notes

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

@thaJeztah thaJeztah added status/2-code-review area/plugins kind/refactor PR's that refactor, or clean-up code labels Mar 18, 2025
@thaJeztah

This comment was marked as outdated.

@thaJeztah
Copy link
Copy Markdown
Member Author

Oh! Ignore me; It's the context that's nil - interesting; I recall we once had a discussion about that, but Cobra itself was expected to set this automatically 🤔

=== RUN   TestListPluginsIsSorted
--- FAIL: TestListPluginsIsSorted (0.00s)
panic: cannot create context from nil parent [recovered]
	panic: cannot create context from nil parent

This error-group was added in 89583b9, but
passed a context.TODO because the function didn't have a context as argument.

However, it does get the root-command passed, which holds the context, so
we can pass that.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Verify that listPluginCandidates returns an empty result if nothing was
found.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Skip the other logic, which includes listing all commands provided; if
there's no plugin-candidates, those steps won't be needed.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function returned an error (if any) from [config.Path]. However, the
only situation in which an error could be returned was if the given path
to append to `config.Dir` was outside of the config directory. This can
only happen if the path to append would try to traverse directories (e.g.,
passing `../../cli-plugins`).

Given that we're passing a hard-coded value, that would not be the case,
so we can simplify the code to join the path directly, and don't have to
handle errors.

[config.Path]: https://github.com/docker/cli/blob/2d74733942be353bc7730c8722ae2414f368b732/cli/config/config.go#L100-L107

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the plugin_manager_cleanups branch from 60ef134 to 091421f Compare March 18, 2025 11:10
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Mar 18, 2025

Codecov Report

Attention: Patch coverage is 61.11111% with 7 lines in your changes missing coverage. Please review.

Project coverage is 59.38%. Comparing base (2d74733) to head (091421f).
Report is 24 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5936      +/-   ##
==========================================
+ Coverage   59.33%   59.38%   +0.04%     
==========================================
  Files         358      358              
  Lines       29783    29776       -7     
==========================================
+ Hits        17672    17681       +9     
+ Misses      11142    11127      -15     
+ Partials      969      968       -1     
🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@thaJeztah thaJeztah requested review from Benehiko, crazy-max and vvoland and removed request for crazy-max March 19, 2025 09:57
Comment on lines +150 to +154
if ctx == nil {
// Fallback, mostly for tests that pass a bare cobra.command
ctx = context.Background()
}
eg, _ := errgroup.WithContext(ctx)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

maybe we should instead force the tests to set a context?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, I originally updated the test, but given that this function is exported, I could not 100% exclude the possibility for other code to call this with a command that has not (yet?) has a context set.

Without a context, this produces a panic at runtime, which was a bit risky, so ultimately I chose for making this "best effort", and keep the test as-is to verify that things work even if no context is present on the command.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah i see, alright then let's get this one merged.

@thaJeztah thaJeztah added this to the 28.0.3 milestone Mar 19, 2025
@thaJeztah thaJeztah merged commit 23eadcd into docker:master Mar 19, 2025
@thaJeztah thaJeztah deleted the plugin_manager_cleanups branch March 19, 2025 15:13
@thaJeztah thaJeztah self-assigned this May 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/plugins 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.

4 participants