-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
feat(cli): add support for multiple sources to sync command #18016
feat(cli): add support for multiple sources to sync command #18016
Conversation
Signed-off-by: ishitasequeira <[email protected]> use arrays instead of map to display ApplicationManifetQuery fields in swagger Signed-off-by: ishitasequeira <[email protected]> rebase and update logic for sync command Signed-off-by: ishitasequeira <[email protected]> update conditions Signed-off-by: ishitasequeira <[email protected]> update displayRevisions on OperationState Signed-off-by: ishitasequeira <[email protected]> remove rerunreport file Signed-off-by: ishitasequeira <[email protected]> fix index 0 out of bounds error Signed-off-by: ishitasequeira <[email protected]> Address comments Signed-off-by: ishitasequeira <[email protected]> fix codegen Signed-off-by: ishitasequeira <[email protected]> rename GetSourcePtrBySourceIndex to GetSourcePtrByIndex Signed-off-by: ishitasequeira <[email protected]> rename GetSourcePtrBySourcePosition to GetSourcePtrByPosition Signed-off-by: ishitasequeira <[email protected]> rebase with master and resolve conflicts Signed-off-by: ishitasequeira <[email protected]> fix codegen Signed-off-by: ishitasequeira <[email protected]> Address feedback and add tests Signed-off-by: ishitasequeira <[email protected]> fix unit test Signed-off-by: ishitasequeira <[email protected]>
Signed-off-by: ishitasequeira <[email protected]>
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.
I made some small comments but I don't see anything worth slowing this down.
Might want a 2nd review on the changes to application.go
as that's more core.
@@ -1849,6 +1851,9 @@ func NewApplicationSyncCommand(clientOpts *argocdclient.ClientOptions) *cobra.Co | |||
argocd app sync -l '!app.kubernetes.io/instance' | |||
argocd app sync -l 'app.kubernetes.io/instance notin (my-app,other-app)' | |||
|
|||
# Sync a multi-source application for specific revision of specific sources | |||
argocd app manifests my-app --revisions 0.0.1 --source-positions 1 --revisions 0.0.2 --source-positions 2 |
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.
Hmm, I wonder if we should have named sources too. Maybe a followup PR, not worth stalling this one.
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.
There is an open issue opened for the same as a future enhancement. #17731
"type": "array", | ||
"items": { | ||
"type": "string", | ||
"format": "int64" |
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.
We could probably use int8, I doubt we'll ever have more than 127 source positions to worry about.
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.
Unfortunately, the command flagset does not support int8
. Kept it int64
to avoid unwanted typecasting between command and api server.
f875931
into
argoproj:release-2.11
Cherry-picking #17808
Checklist: