Skip to content

Conversation

@ijc
Copy link
Contributor

@ijc ijc commented Apr 30, 2019

This regressed in 3af168c ("Ensure plugins can use PersistentPreRunE
again.") but this wasn't noticed because the helloworld test plugin has it's
own PersistentPreRunE which has the effect of deferring the resolution of the
global variable. In the case where the hook isn't used the variable is resolved
during newPluginCommand which is before the global variable was set.

Initialize the plugin command with a stub function wrapping the call to the
(global) hook, this defers resolving the variable until after it has been set,
otherwise the initial value (nil) is used in the struct.

Signed-off-by: Ian Campbell ijc@docker.com

- What I did

- How I did it

- How to verify it

- Description for the changelog

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

This regressed in 3af168c ("Ensure plugins can use `PersistentPreRunE`
again.") but this wasn't noticed because the helloworld test plugin has it's
own `PersistentPreRunE` which has the effect of deferring the resolution of the
global variable. In the case where the hook isn't used the variable is resolved
during `newPluginCommand` which is before the global variable was set.

Initialize the plugin command with a stub function wrapping the call to the
(global) hook, this defers resolving the variable until after it has been set,
otherwise the initial value (`nil`) is used in the struct.

Signed-off-by: Ian Campbell <ijc@docker.com>
@codecov-io
Copy link

Codecov Report

Merging #1853 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1853   +/-   ##
=======================================
  Coverage   56.75%   56.75%           
=======================================
  Files         309      309           
  Lines       21658    21658           
=======================================
  Hits        12292    12292           
  Misses       8469     8469           
  Partials      897      897

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @ijc 🦁

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit 11d2e40 into docker:master May 13, 2019
@GordonTheTurtle GordonTheTurtle added this to the 19.09.0 milestone May 13, 2019
@ijc ijc deleted the cli-plugin-persistentprerun-hook branch May 13, 2019 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants