Skip to content

fix: restore os.Args after plugin completion and fix error return#6817

Merged
thaJeztah merged 1 commit intodocker:masterfrom
luojiyin1987:fix-plugin-cobra
Mar 13, 2026
Merged

fix: restore os.Args after plugin completion and fix error return#6817
thaJeztah merged 1 commit intodocker:masterfrom
luojiyin1987:fix-plugin-cobra

Conversation

@luojiyin1987
Copy link
Contributor

@luojiyin1987 luojiyin1987 commented Feb 24, 2026

- What I did

Fix two issues in the plugin manager:

  1. Fix error return value: The plugin stub was returning the wrong error variable when flag parsing failed
  2. Restore os.Args after plugin completion: The global os.Args was being modified during plugin completion but not restored, which could cause side effects in subsequent code

- How I did it

  1. Changed the error return from err to perr when flag parsing fails
  2. Added code to save the original os.Args before modification and use defer to restore it
  3. Added comprehensive tests to verify both fixes

- Files changed:

File Change
cli-plugins/manager/cobra.go Fix error return + restore os.Args
cli-plugins/manager/cobra_test.go Add regression tests

- How to verify it

The new tests verify both scenarios.

- Description for the changelog

Fix os.Args restoration and error handling in plugin manager

Fixes #6816

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.

whoops! I started reviewing, but didn't submit 🙈

@luojiyin1987
Copy link
Contributor Author

Updated in commit 402b1db and addressed all review points (inline parse error handling, helper returns error, stdlib temp file setup, and config patch simplification). I also resolved the review threads.\n\n@thaJeztah could you please take another look when you have a moment? Thanks!

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

This comment was marked as outdated.

Signed-off-by: luojiyin <luojiyin@hotmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes two bugs in the plugin manager: a wrong error variable return on flag parse failure, and missing restoration of os.Args after plugin completion.

Changes:

  • Fix error return in RunE to use the inline err from flags.Parse instead of the shadowed perr
  • Save and restore os.Args with defer in ValidArgsFunction to prevent side effects
  • Add regression tests for both fixes

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
cli-plugins/manager/cobra.go Fix error variable and add os.Args save/restore
cli-plugins/manager/cobra_test.go Add tests for parse error return and os.Args restoration

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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

Copy link
Contributor

@austinvazquez austinvazquez 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 26d4525 into docker:master Mar 13, 2026
107 checks passed
@luojiyin1987 luojiyin1987 deleted the fix-plugin-cobra branch March 14, 2026 00:41
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.

fix: restore os.Args after plugin completion and fix error return

5 participants