fix(hydrator): use refresh paths from drySource when source hydration is enabled #25516
Conversation
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
|
I'll override DCO if it becomes a huge pain to fix. :-) |
FourFifthsCode
force-pushed
the
hydrator-refresh-fix
branch
from
3c63671 to
b1d4286
Compare
I rebased on upstream and just cherry picked @pbhatnagar-oss's commits |
agaudreault
left a comment
agaudreault
left a comment
There was a problem hiding this comment.
I think after GetAppRefreshPaths is changed to receive the source in parameters, the implementation of the webhook handle event is much simpler.
if the app has an hydrator source configured, check the DrySource for change.
then, check the default source for change.
0eade53 to
0029f3b
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #25516 +/- ##
==========================================
+ Coverage 62.61% 62.65% +0.04%
==========================================
Files 353 353
Lines 50159 50181 +22
==========================================
+ Hits 31405 31440 +35
+ Misses 15737 15721 -16
- Partials 3017 3020 +3 ☔ View full report in Codecov by Sentry. |
util/webhook/webhook.go
Outdated
| } else if sourceRevisionHasChanged(syncSource, revision, touchedHead) && sourceUsesURL(syncSource, webURL, repoRegexp) { // handle webhook for syncSource | ||
| if err := a.storePreviouslyCachedManifests(&app, change, trackingMethod, appInstanceLabelKey, installationID); err != nil { | ||
| log.Warnf("Failed to store cached manifests of previous revision for app '%s': %v", app.Name, err) | ||
| } | ||
|
|
||
| refreshPaths := path.GetAppRefreshPaths(&app, syncSource) | ||
| if path.AppFilesHaveChanged(refreshPaths, changedFiles) { | ||
| // syncSource changes trigger sync (no hydration needed), but still filter irrelevant files | ||
| namespacedAppInterface := a.appClientset.ArgoprojV1alpha1().Applications(app.Namespace) | ||
| log.Infof("webhook trigger refresh app from syncSource '%s'", app.Name) | ||
| _, err = argo.RefreshApp(namespacedAppInterface, app.Name, v1alpha1.RefreshTypeNormal, true) | ||
| if err != nil { | ||
| log.Warnf("Failed to refresh app '%s' after syncSource change: %v", app.Name, err) | ||
| continue | ||
| } | ||
| } |
There was a problem hiding this comment.
This does not seem necessary since app.Spec.GetSources() handles the hydrator syncSource below.
| } | ||
|
|
||
| refreshPaths := path.GetAppRefreshPaths(&app, syncSource) | ||
| if path.AppFilesHaveChanged(refreshPaths, changedFiles) { |
There was a problem hiding this comment.
if path.AppFilesHaveChanged is false, then it needs to do the same as the loop below to update the cache. So it needs to call storePreviouslyCachedManifests for the current source.
storePreviouslyCachedManifests will require a source to be added to its param to know on which source it is currently acting.
There was a problem hiding this comment.
moved logic so that we only need to have code that calls RefreshApp/storePreviouslyCachedManifests once based on booleans. How does that look to you?
| hook.Reset() | ||
| } | ||
|
|
||
| func TestGitHubCommitEvent_SyncSourceRefresh(t *testing.T) { |
There was a problem hiding this comment.
The unit tests should validate that SetNewRevisionManifests has been called when the source matches, but the path do not match any changed files
There was a problem hiding this comment.
Seems like we can just check if the refresh/hydrate annotations were correctly applied since that is the result of RefreshApp function being called.
… is enabled Signed-off-by: Codey Jenkins <[email protected]>
Signed-off-by: Codey Jenkins <[email protected]>
…r is enabled Signed-off-by: pbhatnagar-oss <[email protected]>
Signed-off-by: pbhatnagar-oss <[email protected]>
Signed-off-by: Codey Jenkins <[email protected]>
Signed-off-by: Codey Jenkins <[email protected]>
…on dry or sync sources Signed-off-by: Codey Jenkins <[email protected]>
Add AppFilesHaveChanged check to syncSource webhook handling and fix GetAppRefreshPaths to return source path for drySource without annotation. This ensures consistent file filtering behavior for both dry and sync sources. Signed-off-by: Codey Jenkins <[email protected]>
Use .Equals() instead of == when comparing source to syncSource in annotation processing. The == comparison always failed since GetSyncSource() returns a new struct each time. Signed-off-by: Codey Jenkins <[email protected]>
…ource sources Signed-off-by: Codey Jenkins <[email protected]>
Hydration with sourceHydrator: syncSource refresh - Direct sync from hydrated branch without re-hydration drySource with annotation - Hydration triggered only when changed files match manifest-generate-paths drySource without annotation - Hydration triggered when changed files are within drySource path (default fallback) Standard applications (no sourceHydrator): With annotation - Refresh only when changed files match manifest-generate-paths Without annotation - Always refresh on any change (default behavior) Signed-off-by: Codey Jenkins <[email protected]>
FourFifthsCode
force-pushed
the
hydrator-refresh-fix
branch
from
41c1729 to
2038e9f
Compare
Signed-off-by: Codey Jenkins <[email protected]>
4 participants
When using
argocd.argoproj.io/manifest-generate-pathsannotation with source hydrator enabled apps, make sure to base relative paths off of drySource path and not the sync source.This fix allows hydration to properly trigger refreshes on paths specified by manifest-generate-paths annotation.
Hydration with sourceHydrator:
Standard applications (no sourceHydrator):
Partial fix for #25094
Checklist: