fix(hydrator): use refresh paths from drySource when source hydration is enabled #25516
Conversation
❗ Preview Environment deployment failed on BunnyshellSee: Environment Details | Pipeline Logs Available commands (reply to this comment):
|
util/app/path/path.go
Outdated
| // For sourceHydrator apps, resolve paths relative to the DRY source | ||
| // since webhook events contain files changed in the dry source repository |
There was a problem hiding this comment.
You also want to resolve the path relative to the syncSource, otherwise the application will not sync.
There was a problem hiding this comment.
@agaudreault I think that's addressed here: #25295 Maybe we should combine the PRs?
There was a problem hiding this comment.
combining sounds good to me
There was a problem hiding this comment.
Yeah, I think we should have the complete fix in 1 PR so it is easier to understand.
This PR alone would make it worse I think, because it is more important to sync fast the desired state than hydrate fast IMO.
There was a problem hiding this comment.
I have rebased on @pbhatnagar-oss's PR and added paths for both sync and dry sources
|
I'll override DCO if it becomes a huge pain to fix. :-) |
… is enabled Signed-off-by: Codey Jenkins <[email protected]>
Signed-off-by: Codey Jenkins <[email protected]>
FourFifthsCode
force-pushed
the
hydrator-refresh-fix
branch
from
3c63671 to
b1d4286
Compare
…r is enabled Signed-off-by: pbhatnagar-oss <[email protected]>
Signed-off-by: pbhatnagar-oss <[email protected]>
I rebased on upstream and just cherry picked @pbhatnagar-oss's commits |
Signed-off-by: Codey Jenkins <[email protected]>
Signed-off-by: Codey Jenkins <[email protected]>
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.
| continue | ||
| } | ||
| } | ||
| } else if sourceRevisionHasChanged(syncSource, revision, touchedHead) && sourceUsesURL(syncSource, webURL, repoRegexp) { // handle webhook for syncSource |
There was a problem hiding this comment.
Should call storePreviouslyCachedManifests if no changes in the revision.
| for _, source := range app.Spec.GetSources() { | ||
| paths = append(paths, filepath.Clean(filepath.Join(source.Path, item))) | ||
| // For sourceHydrator apps, resolve paths relative to DRY source | ||
| if app.Spec.SourceHydrator != nil { |
There was a problem hiding this comment.
Should receive the current source in parameter since it influence the refresh path.
- if source hydrator configured, if syncing sources, use sync source path
- if source hydrator configured, if hydrating sources, use annotation path and Dry source (source in params) base
- if source hydrator not configured, use annotation path and default source base (source in params)
| } else if sourceRevisionHasChanged(syncSource, revision, touchedHead) && sourceUsesURL(syncSource, webURL, repoRegexp) { // handle webhook for syncSource | ||
| // syncSource webhook events only trigger sync, not hydration. Unlike drySource events, | ||
| // skip the manifest_generate_path file check since hydration is not required here. | ||
| namespacedAppInterface := a.appClientset.ArgoprojV1alpha1().Applications(app.Namespace) |
There was a problem hiding this comment.
This should also check if AppFilesHaveChanged.
| } else { | ||
| for _, source := range app.Spec.GetSources() { | ||
| paths = append(paths, filepath.Clean(filepath.Join(source.Path, item))) | ||
| // For sourceHydrator apps, resolve paths relative to DRY source |
There was a problem hiding this comment.
This method is also called in the GetRepoObjs function and should return the right value depending if we are current acting on the dry or sync source.
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.
Partial fix for #25094
Checklist: