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

Support supplying flags like Prune in the app updater when requesting a sync from ArgoCD #2984

Open
1 of 3 tasks
andrii-korotkov-verkada opened this issue Nov 23, 2024 · 6 comments

Comments

@andrii-korotkov-verkada

Checklist

  • I've searched the issue queue to verify this is not a duplicate feature request.
  • I've pasted the output of kargo version, if applicable.
  • I've pasted logs, if applicable.

Proposed Feature

ArgoCD can pass extra options to sync, e.g. Prune https://github.com/argoproj/argo-cd/blob/32cc6638f78e15652c24eb2cd6403e6e7ec26ab0/server/application/application.go#L1971. Kargo doesn't set Prune, yet sets SyncOptions

Sync: &argocd.SyncOperation{
Revisions: []string{},
},
}
if app.Spec.SyncPolicy != nil {
if app.Spec.SyncPolicy.Retry != nil {
app.Operation.Retry = *app.Spec.SyncPolicy.Retry
}
if app.Spec.SyncPolicy.SyncOptions != nil {
app.Operation.Sync.SyncOptions = app.Spec.SyncPolicy.SyncOptions
}
}
if app.Spec.Source != nil {
app.Operation.Sync.Revisions = []string{app.Spec.Source.TargetRevision}
}
for _, source := range app.Spec.Sources {
app.Operation.Sync.Revisions = append(app.Operation.Sync.Revisions, source.TargetRevision)
}
. It be nice to allow specifying Prune with update requests.

Motivation

Parity with ArgoCD sync.

Suggested Implementation

Surface Prune checkbox in UI if not already, then pass the flag to the sync field in the operation.

@krancour
Copy link
Member

krancour commented Dec 4, 2024

flags like Prune

Prune seems like a sensible option to expose. If there are others you feel would be especially useful, please enumerate.

I don't want to blindly expose all options, as my gut says not all of them make sense in the context of Kargo.

Prune, at least, is absolutely doable.

@krancour krancour added help-wanted Community help on this would be appreciated good first issue Good issue for a new contributor to handle and removed needs/area kind/proposal labels Dec 4, 2024
@andrii-korotkov-verkada
Copy link
Author

There also can be Replace, Force, Dry Run. Regarding Replace, there's this proposal argoproj/argo-cd#20730, that would be discussed soon.

@krancour
Copy link
Member

krancour commented Dec 4, 2024

Rather than exposing any of these as options in the argocd-update step, I'm wondering if these are just things that should be copied over from the Application's sync policy when we create the sync operation.

Is there any reason that the options used by argocd-update should differ from what's in the sync policy?

cc @jessesuen for input.

@krancour krancour added needs-discussion and removed good first issue Good issue for a new contributor to handle help-wanted Community help on this would be appreciated labels Dec 9, 2024
@jessesuen
Copy link
Member

I'm wondering if these are just things that should be copied over from the Application's sync policy when we create the sync operation.

Actually, I already assumed we were doing that, so we should make that change. I believe Argo CD auto-sync will pick up the applications preferences when creating the operation, so we can copy that logic.

@jessesuen
Copy link
Member

Sync flags might be good to support in the argocd-update step too, in case there is a use case to override the Application's default sync settings.

I don't want to blindly expose all options

I believe because sync options are just generic string, we just need to support a string field and pass it on. There would be no upkeep of supporting more and more options.

@krancour
Copy link
Member

Actually, I already assumed we were doing that, so we should make that change.

I thought there were, perhaps, things we ought to be copying but weren't but I just double-checked and we are already copying all sync options.

So @andrii-korotkov-verkada, if you have specific sync options set on the App, Kargo will already use them.

Sync flags might be good to support in the argocd-update step too, in case there is a use case to override the Application's default sync settings.

I believe because sync options are just generic string, we just need to support a string field and pass it on. There would be no upkeep of supporting more and more options.

I agree, especially if it's this straightforward.

Thanks @jessesuen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants