feat: add cli commands to create/set/unset/edit sources for multi-source application

[ishitasequeira]

The PR focusses on 4 commands:

• argocd app create - The current app create command works with file option for creating multi-source apps but overrides parameters only for 1st source while specifying additional options. The PR allows users to provide source index for overriding parameters for a specific source.

Example command:

$ ./dist/argocd app create --file ~/app1.yaml --repo https://github.com/ishitasequeira/argocd-example-apps.git --source-index 1
application 'guestbook' created

• argocd app set - The current app set command does not allow to set and override parameters for a multi-source application. The PR updates the command to be able to set overrides for a specific source index.

Example command:

$ ./dist/argocd app set guestbook --source-index 1 --ref values --repo https://github.com/ishitasequeira/argocd-example-apps.git

• argocd app unset - The current app unset command does not allow to set and override parameters for a multi-source application. The PR updates the command to be able to unset overrides for a specific source index.

Example command:

$ ./dist/argocd app unset guestbook --source-index 1 --namesuffix

• argocd app edit - The current app edit command does not allow to override parameters for a multi-source application while editing the application. The PR updates the command to be able to edit overrides for a specific source index.

Example command:

$ ./dist/argocd app edit guestbook --source-index 1 --repo https://github.com/ishitasequeira/argocd-example-apps.git

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[jannfis]

I think from a UX perspective (principle of least surprise), it would make sense that when an app has multiple sources defined and --source-index is not specified, that the request will fail.

E.g.

./dist/argocd app create --file ~/app1.yaml --repo https://github.com/ishitasequeira/argocd-example-apps.git

will fail instead of assuming the default source index to be 0.

This might prevent the one or other accident.

[ishitasequeira]

That does make sense.

I defaulted it to 0 for maintaining today's behavior. I will update it to error out.

[codecov]

Codecov Report

Attention: Patch coverage is 16.49485% with 81 lines in your changes are missing coverage. Please review.

Project coverage is 49.39%. Comparing base (05eea87) to head (8ef0fc9).
Report is 30 commits behind head on master.

❗ Current head 8ef0fc9 differs from pull request most recent head 3520132. Consider uploading reports for the commit 3520132 to get more accurate results

Files	Patch %	Lines
cmd/argocd/commands/app.go	8.69%	62 Missing and 1 partial ⚠️
cmd/util/app.go	43.47%	12 Missing and 1 partial ⚠️
pkg/apis/application/v1alpha1/types.go	0.00%	4 Missing ⚠️
cmd/argocd/commands/admin/app.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #17425      +/-   ##
==========================================
- Coverage   49.44%   49.39%   -0.05%     
==========================================
  Files         273      273              
  Lines       48662    48735      +73     
==========================================
+ Hits        24063    24075      +12     
- Misses      22237    22297      +60     
- Partials     2362     2363       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[ishitasequeira]

@jannfis I have updated the code for commands edit, set and unset to error out when source_index is not provided for multi-source apps.
Although, it is a bit difficult to do the same for create command. The current flow of create command when specifying --file option, by default calls SetAppSpecOptions to iterate over all flags/appOptions to set the default values if any flag is specified. I could not find a way to find the length of flags specified to see if any flag other than source_index was set.

Thus, left the create command with source_index defaulting to 0.

[jannfis]

Thus, left the create command with source_index defaulting to 0.

I was wondering if it makes sense to support manipulating the source for the create command at all, or if the create command should abort if the app a) has multiple sources and b) is templated from a YAML and c) one of the source modificators, such as --repo, is specified.

AFAIU (keep me honest here), it would only be possible for the create command to override a single source definition for a given app (as specified by --source-index), but not modify multiple app sources. As opposed to edit or set, create cannot be called multiple times using a different --source-index for the same application.

I haven't looked yet, but is the app create command able to generate a multi-source application without a YAML template right now?

[ishitasequeira]

Thus, left the create command with source_index defaulting to 0.

I was wondering if it makes sense to support manipulating the source for the create command at all, or if the create command should abort if the app a) has multiple sources and b) is templated from a YAML and c) one of the source modificators, such as --repo, is specified.

I had the same question initially and I am still open to stop allowing to override parameters. I kept it to update for allowing the behavior supported today.

AFAIU (keep me honest here), it would only be possible for the create command to override a single source definition for a given app (as specified by --source-index), but not modify multiple app sources. As opposed to edit or set, create cannot be called multiple times using a different --source-index for the same application.

Yes, the user will be able to override the definition only for a single source specified by source-index.

I haven't looked yet, but is the app create command able to generate a multi-source application without a YAML template right now?

Yes, the multi-source application can be created only with YAML configuration (that is with fileURL specified).

[jannfis]

I'm not sure if this is possible with cobra, but do you think we can have dynamic command line arguments? Then instead of --repo <url> --source-index X, we could have --repoX (where X specifies the index of the source), so

--repo foo --source-index 0

would become

--repo0 foo

We'd be able to override all repository URLs atomically within a single command execution, for both create and edit.

For this, we wouldn't define the parameters statically but see at runtime whether that flag would be valid (e.g. --repo127 obviously wouldn't be valid if the application only has 125 sources).

Not sure if that's cool or even worth the hassle, though.

[ishitasequeira]

I recall not opting for implementing dynamic flags while developing the ApplicationSet CLI. This decision was mainly driven by concerns about having huge list of command arguments (in this case it would be for defining multiple sources). Also, it would be difficult to validate the application source definition for missing properties.

I think it would be good to begin with not allowing overrides. We can see if there is any ask for overrides from the community and come to a consensus around it's implementation. WDYT?

[jannfis]

Yep, I'm cool with that!

[ishitasequeira]

@jannfis I have updated the PR.

[jannfis]

First round, mostly nits and style concerns.

[jannfis]

Just a nit, but this should be sourceIndex instead of source_index (also, throughout all other files, too)

[jannfis]

I was wondering if it would confusing be for the majority of users that the index starts at 0. For programmers, it might feel natural to start counting at 0, but the majority of people probably start counting at 1 🙂

[ishitasequeira]

Updated it to start counting at 1 😅

[jannfis]

👍

[jannfis]

Suggested change

[jannfis]

!needToUnsetRef && true somehow reads (and possibly is) redundant

[jannfis]

Another one :)

[jannfis]

Reading this, I will assume that just by pointing to a YAML would give me a multi source app, but I think this is not true.

[ishitasequeira]

It is true given that the yaml file contains application configured with multiple sources. I will specify that in the example.

e.g.:

$ ./dist/argocd app create --file ~/app1.yaml 
application 'guestbook' created

[ishitasequeira]

@jannfis Addressed all your comments.

[anandf]

LGTM

[jannfis]

LGTM, thanks @ishitasequeira !