-
Notifications
You must be signed in to change notification settings - Fork 289
feat: Add sync-dependencies #514
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
Conversation
Signed-off-by: Alex Collins <alex_collins@intuit.com>
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.
In syncTasks
we need to check for cyclic-dependencies.
// delete all completed hooks which have appropriate delete policy | ||
sc.deleteHooks(hooksPendingDeletionSuccessful) | ||
sc.setOperationPhase(common.OperationSucceeded, "successfully synced (all tasks run)") | ||
} else { | ||
sc.setRunningPhase(remainingTasks, false) | ||
sc.setRunningPhase(tasks, false) |
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.
Looks like a 2+ year old bug.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #514 +/- ##
==========================================
- Coverage 55.75% 55.61% -0.15%
==========================================
Files 41 42 +1
Lines 4525 4531 +6
==========================================
- Hits 2523 2520 -3
- Misses 1808 1817 +9
Partials 194 194 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Signed-off-by: Alex Collins <alex_collins@intuit.com>
sc.log.WithValues("tasks", tasks).V(1).Info("tasks after sorting") | ||
|
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.
sc.log.WithValues("tasks", tasks).V(1).Info("tasks after sorting") |
Kudos, SonarCloud Quality Gate passed!
|
|
I wonder whether we can hit a scenario of a dependent resource that has an auto-generated and thus unknown ahead of time name? |
Is it possible to fix CI? |
Hey @alexec - I have a feeling that this would be a major gamechanger in syncing large changes with fine-grain dependencies. Is there anything we can do to help this get pushed? What are the outstanding issues? We can help in coding/review - whatever needed. |
@or-shachar see discussion here: argoproj/argo-cd#7437 I don't think there's been sufficient work done on the proposal side to show that the feature is necessary/justified or describing the details of how it will function. Gonna close this for now, since I don't think Alex is actively pursuing the feature. |
Depended on by argo-cd#3517
This PR introduces a new feature: sync dependencies.
Sync dependencies give a new way to order sync operations that is influenced by sync waves and sync hooks (all of which I wrote, so I know it extremely well).
Like sync waves, a sync operation will only progress when all the dependents have completed.
Dependencies between objects is specified by the new
argocd.argoproj.io/sync-dependencies
annotation.Reasons to do this:
Reasons not to do this:
Open questions: