Skip to content
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

fix(cmp): discover plugins relative to app path (#13940) #13946

Conversation

crenshaw-dev
Copy link
Member

@crenshaw-dev crenshaw-dev commented Jun 7, 2023

According to our docs, CMP discovery was always meant to be relative to the repo root. That was a reasonable design, because discovery for other config management tools (e.g. Helm) is done at the repo root.

But those other tools have multiple outputs: the names of the directories which they can handle. CMP discovery only has a boolean output: "yes, I support that directory" or "no, I do not support that directory."

So, the actual implementation (in contradiction to the docs) answers that question at the app's path instead of the repo root. That's reasonable, because Argo CD needs to know "will this plugin work in this directory?" Since we only get a yes/no, we have to ask the question about a specific directory, instead of about the whole repo.

#13360 made one important fix: instead of just sending the app directory to the plugin, we send the whole repo. That makes sense, because stuff outside the app directory might be important.

But the PR made a mistake: it changed the discovery mechanism to evaluate discovery rules relative to the repo root instead of the app path.

This PR does two things:

  1. It returns CMP discovery to be relative to the app path instead of repo root.
  2. It corrects the docs.

This PR does not revert the changes from #13360 which makes the whole repo available to the plugin.

Fixes #13940

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev crenshaw-dev marked this pull request as ready for review June 11, 2023 18:07
@crenshaw-dev
Copy link
Member Author

/cherry-pick release-2.7

@crenshaw-dev
Copy link
Member Author

/cherry-pick release-2.6

cmpserver/plugin/plugin.go Outdated Show resolved Hide resolved
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Copy link
Collaborator

@leoluz leoluz left a 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: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev crenshaw-dev enabled auto-merge (squash) June 15, 2023 17:50
@codecov
Copy link

codecov bot commented Jun 15, 2023

Codecov Report

Patch coverage: 28.00% and no project coverage change.

Comparison is base (3c6a6e1) 49.56% compared to head (809ca8d) 49.56%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #13946   +/-   ##
=======================================
  Coverage   49.56%   49.56%           
=======================================
  Files         256      256           
  Lines       43920    43927    +7     
=======================================
+ Hits        21770    21774    +4     
- Misses      19987    19990    +3     
  Partials     2163     2163           
Impacted Files Coverage Δ
util/cert/cert.go 76.56% <0.00%> (ø)
util/git/creds.go 50.93% <0.00%> (ø)
util/gpg/gpg.go 66.00% <0.00%> (ø)
util/helm/cmd.go 31.65% <0.00%> (ø)
util/app/discovery/discovery.go 34.78% <25.00%> (ø)
cmpserver/plugin/plugin.go 79.64% <46.15%> (-1.65%) ⬇️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@crenshaw-dev crenshaw-dev merged commit cec07ee into argoproj:master Jun 15, 2023
@gcp-cherry-pick-bot
Copy link

Cherry-pick failed with Merge error cec07ee1ceecdb31ca9c0cd7e350a9b1041de74a into temp-cherry-pick-0b153a-release-2.7

@gcp-cherry-pick-bot
Copy link

Cherry-pick failed with Merge error cec07ee1ceecdb31ca9c0cd7e350a9b1041de74a into temp-cherry-pick-0b153a-release-2.6

@crenshaw-dev crenshaw-dev deleted the discover-plugins-relative-to-app-path branch June 15, 2023 21:36
crenshaw-dev added a commit to crenshaw-dev/argo-cd that referenced this pull request Jun 15, 2023
…oproj#13946)

* fix(cmp): discover plugins relative to app path (argoproj#13940)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* securejoin

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* intuitive constant names

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* comments

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* add missing import

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
crenshaw-dev added a commit to crenshaw-dev/argo-cd that referenced this pull request Jun 15, 2023
…oproj#13946)

* fix(cmp): discover plugins relative to app path (argoproj#13940)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* securejoin

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* intuitive constant names

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* comments

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* add missing import

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
crenshaw-dev added a commit that referenced this pull request Jun 16, 2023
…4084)

* fix(cmp): discover plugins relative to app path (#13940)



* securejoin



* intuitive constant names



* comments



* add missing import



---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
crenshaw-dev added a commit that referenced this pull request Jun 16, 2023
…4085)

* fix(cmp): discover plugins relative to app path (#13940)



* securejoin



* intuitive constant names



* comments



* add missing import



---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
schakrad pushed a commit to schakrad/argo-cd that referenced this pull request Jul 24, 2023
…oproj#13946) (argoproj#14084)

* fix(cmp): discover plugins relative to app path (argoproj#13940)

* securejoin

* intuitive constant names

* comments

* add missing import

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: schakrad <58915923+schakrad@users.noreply.github.com>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
…oproj#13946)

* fix(cmp): discover plugins relative to app path (argoproj#13940)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* securejoin

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* intuitive constant names

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* comments

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* add missing import

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this pull request Dec 16, 2023
…oproj#13946)

* fix(cmp): discover plugins relative to app path (argoproj#13940)

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* securejoin

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* intuitive constant names

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* comments

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

* add missing import

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

v2.7.4 CMP regression: the plugin isn't called
2 participants