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: multi-source app refresh (#11772) #12217

Merged
merged 5 commits into from Feb 6, 2023

Conversation

crenshaw-dev
Copy link
Collaborator

Fixes #11772

This turns out to be gnarlier than I'd hoped.

Basically, the manifest cache for a source is currently based on only the commit hash of the source, not any other sources it references.

I'm adding code to inject the other sources' commits in the cache key.

This won't help webhooks. There's currently no way for a webhook from referenced-repo-Y to trigger a refresh on referencing-source-X. I'm leaving that fix for another PR.

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

codecov bot commented Jan 31, 2023

Codecov Report

Base: 47.43% // Head: 47.47% // Increases project coverage by +0.03% 🎉

Coverage data is based on head (5b3152c) compared to base (adb4471).
Patch coverage: 74.22% of modified lines in pull request are covered.

❗ Current head 5b3152c differs from pull request most recent head 82785ef. Consider uploading reports for the commit 82785ef to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12217      +/-   ##
==========================================
+ Coverage   47.43%   47.47%   +0.03%     
==========================================
  Files         246      246              
  Lines       41824    41927     +103     
==========================================
+ Hits        19838    19903      +65     
- Misses      19990    20022      +32     
- Partials     1996     2002       +6     
Impacted Files Coverage Δ
util/webhook/webhook.go 68.28% <0.00%> (ø)
reposerver/repository/repository.go 60.49% <71.01%> (-0.11%) ⬇️
reposerver/cache/cache.go 60.20% <95.83%> (+0.20%) ⬆️
util/app/discovery/discovery.go 36.09% <0.00%> (-1.12%) ⬇️
util/db/repository_secrets.go 68.35% <0.00%> (-0.44%) ⬇️
cmpserver/plugin/plugin.go 78.51% <0.00%> (-0.34%) ⬇️
server/repository/repository.go 51.49% <0.00%> (-0.13%) ⬇️
cmd/util/repo.go 0.00% <0.00%> (ø)
util/exec/exec.go 100.00% <0.00%> (ø)
util/settings/settings.go 49.20% <0.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev crenshaw-dev marked this pull request as ready for review January 31, 2023 20:17
Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

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

Tested the PR locally and it is working as expected. Just a minor nit.

reposerver/repository/repository.go Show resolved Hide resolved
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Copy link
Member

@ishitasequeira ishitasequeira 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
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.

As discussed with @crenshaw-dev, there is room for improvement in the current PR to avoid adding an additional parameter to an already big method signature in Cache.GetManifests. The agreement was to add TODO comments in the code so we can unblock 2.6 release and address this refactoring later.
LGTM

reposerver/cache/cache.go Show resolved Hide resolved
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
@crenshaw-dev crenshaw-dev merged commit 212029d into argoproj:master Feb 6, 2023
@crenshaw-dev crenshaw-dev deleted the fix-multi-source-refresh branch February 6, 2023 18:02
crenshaw-dev added a commit that referenced this pull request Feb 6, 2023
* fix multi-source refresh

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

* serialize nil and empty resolvedRevisions the same to avoid cache misses

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

* more consistent naming

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

* document duplication

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

* add todo

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
Copy link
Collaborator Author

Cherry-picked onto release-2.6 for 2.6.0.

schakrad pushed a commit to schakrad/argo-cd that referenced this pull request Mar 14, 2023
* fix multi-source refresh

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

* serialize nil and empty resolvedRevisions the same to avoid cache misses

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

* more consistent naming

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

* document duplication

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

* add todo

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

---------

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Signed-off-by: schakrad <chakradari.sindhu@gmail.com>
@julien-deruere
Copy link

Hi Folks, since this exact commit happened, we cannot upgrade our ArgoCD to more than 2.5.22. Our apps are synced but never (hard)refreshed first. It's happening only on one of our cluster, do you have an explanation?

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.

2.6 RC1: multi source application needs a hard refresh to deploy changes
4 participants