-
Notifications
You must be signed in to change notification settings - Fork 6.3k
feat(cli): Add Plugin Support to the Argo CD CLI #20074
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
feat(cli): Add Plugin Support to the Argo CD CLI #20074
Conversation
❗ Preview Environment undeploy from Bunnyshell failedSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #20074 +/- ##
==========================================
+ Coverage 56.08% 59.94% +3.85%
==========================================
Files 343 344 +1
Lines 57546 57656 +110
==========================================
+ Hits 32275 34561 +2286
+ Misses 22627 20335 -2292
- Partials 2644 2760 +116 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please consider adding a new documentation page explaining how to write plugins and how to install and consume them.
92967cd
to
4a260dd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
plugin_test.go
is kept in separate package because keeping it in the util will create circular dependency.
Update: Resolved
0e8f338
to
c314e43
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nitishfy Great progress. I think this PR needs to be reviewed by someone from the security sig as I see some potential risks in the code. I added comments to the security issues that I identified at first but a more security driven review would be important in this case.
cc @jannfis @crenshaw-dev
652887c
to
fea85cd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not using the cmd.Find()
anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check my comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL.
Also, I think I'll have to restructure the entire tests for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great progress. Tks @nitishfy!
There are a few more open items to be addressed in this PR before we merge.
Please take a look.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits, otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PTAL.
cmd/argocd/commands/plugin.go
Outdated
// set by the cobra.OnInitialize() was never executed because cmd.Execute() | ||
// gave us a non-nil error. | ||
initConfig() | ||
log.SetLevel(log.DebugLevel) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry the previous comment got deleted...
the log level needs to be setup manually here since the initConfig()
set by the cobra.OnInitialize()
was never executed because cmd.Execute()
gave us a non-nil error.
cc @agaudreault |
Update: There's a little issue related to flag parsing that needs to be worked upon. Rn, all the global level flags won't be parsed when you are running your plugin. This is happening because cobra internally doesn't parse the flags when a command is not found. This also means that if a user has set its own flags in a binary that matches with Argo CD global flags, the default value of the Argo CD global flags will be used no matter what (why? because flag parsing never happens by cobra for an unknown command). For normal Argo CD commands, the flag parsing and everything else remains same as this is how it used to be. What should be a better approach to handle this? Should we add this in a limitation section and tell users about not to use duplicate flags as argocd global flags? Do we even want a user to set it's own flags that overrides argocd global flag values?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should be a better approach to handle this? Should we add this in a limitation section and tell users about not to use duplicate flags as argocd global flags? Do we even want a user to set it's own flags that overrides argocd global flag values?
@nitishfy In my view, we need to guarantee to plugin writers 2 capabilities:
- The plugin doesn't have to authenticate with Argo CD API. Plugin authors can directly invoke the Argo CD API and the current context will be used.
- While some global flags don't make sense to be used with plugins, some others do:
--argocd-context
,--auth-token
,--logformat
,--loglevel
are some (but not the only ones) that makes sense to be available when executing plugins. However I don't think that it is the plugin responsibility to handle those flags. We would need to pre-process the flags before invoking the plugin itself.
That being said, it seems that there is more work required in order to get the plugin feature ready. However I am not opposed if other maintainers are ok with moving forward clearly documenting the current limitations and marking the feature as alpha.
Could we not have a dummy (no-op) command for the error path? We invoke the dummy command that will parse the flags, specifically the flags we would like to still keep (we can use the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discusssions at KubeCon, I'm proposing few things here:
- A plugin should always respect it's own flags and it's a default behaviour if it doesn't respect the global flags.
- Although we may not require all flags for parsing, this actually can't be done because cobra internally doesn't parse flags if a command doesn't exist. Kubectl does the similar way. For example, If I have a kubectl plugin, a global flag wouldn't be parsed such as:
kubectl demo -h # unknown flag '-h'
TLDR: A Plugin should respect it's own flag and we should properly document this in the iimitations section that when you're creating your custom plugins, the global flag won't be parsed similar to how you can't create your own subcommand that override the existing argocd commnds.
Signed-off-by: nitishfy <justnitish06@gmail.com> modify mkdocs.yaml Signed-off-by: nitishfy <justnitish06@gmail.com> add unit test Signed-off-by: nitishfy <justnitish06@gmail.com> add more tests Signed-off-by: nitishfy <justnitish06@gmail.com> modify docs a bit Signed-off-by: nitishfy <justnitish06@gmail.com> update docs Signed-off-by: nitishfy <justnitish06@gmail.com> fix merge conflicts Signed-off-by: nitishfy <justnitish06@gmail.com> fix go.mod Signed-off-by: nitishfy <justnitish06@gmail.com> add line Signed-off-by: nitishfy <justnitish06@gmail.com> fix go.mod Signed-off-by: nitishfy <justnitish06@gmail.com> fix lint checks Signed-off-by: nitishfy <justnitish06@gmail.com> fix failing tests Signed-off-by: nitishfy <justnitish06@gmail.com> fix lint checks Signed-off-by: nitishfy <justnitish06@gmail.com> add e2e tests Signed-off-by: nitishfy <justnitish06@gmail.com> add one more e2e test Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
@leoluz I think the plugin is working as expected, I just tested it for this scenario:
This means once you've logged in to the server using the CLI, the current context will be used by the Plugin unless you change the context explicitly. @agaudreault FYI
I've explicitly set the log format to |
Signed-off-by: nitishfy <justnitish06@gmail.com>
8e62673
to
ae2177b
Compare
Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
Ping @agaudreault |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: nitishfy <justnitish06@gmail.com> Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Signed-off-by: nitishfy <justnitish06@gmail.com> Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Kanika Rana <krana@redhat.com>
Signed-off-by: nitishfy <justnitish06@gmail.com> Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com> Signed-off-by: Oliver Gondža <ogondza@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com> Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Ref: #19624
Checklist: