feat(app): Add optional 'name' to Source object #20470

CefBoud · 2024-10-20T16:45:55Z

Description

This PR adds an optional 'name' to the Application Source object. It can be used instead of the source position in multi-source Applications CLI.

Screenshots

Checklist:

bunnyshell · 2024-10-20T16:46:01Z

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

🚀 /bns:deploy to deploy the environment

nitishfy

If this PR is ready for review, can you go ahead and fix the CI checks?

agaudreault

Good work 👍 The code changes are missing unit tests and maybe e2e too on the different commands. @ishitasequeira has more knowledge on the usage of multi-source. Do you see something that has been forgotten in this PR?

cmd/util/app.go

test/e2e/app_multiple_sources_test.go

ishitasequeira

@CefBoud Thanks for taking this up!! Left some comments. Also, I think this change would also need to be reflected on the Application Create and AddSource UI Pages too.

ishitasequeira · 2024-10-30T11:40:40Z

cmd/argocd/commands/app.go

@@ -381,6 +396,19 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
 			})
 			errors.CheckError(err)

+			if sourceName != "" && sourcePosition != -1 {


Since the Name parameter has been added to the ApplicationSource object, it's not exclusive to multi-source applications. This means the name field also applies to single-source apps. Therefore, in all commands where we’re adding the sourceName check, we should ensure the command supports single-source applications as well, using the sourceName field.

@ishitasequeira Thank you for the review.
My reasoning was that although the field is defined in ApplicationSource, much like Ref and the implicit source-position, it only should be considered in the multi-source case. Additionally, the purpose of the proposal was to offer an alternative to source-position in the multi-source case.
What are your thoughts?

That works for me. Although, let's update the comment on the Name field in types.go to reflect that it is mainly used for multi-source apps.

ishitasequeira · 2024-10-30T11:42:41Z

cmd/argocd/commands/app.go

@@ -435,6 +463,7 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
 	command.Flags().BoolVar(&hardRefresh, "hard-refresh", false, "Refresh application data as well as target manifests cache")
 	command.Flags().StringVarP(&appNamespace, "app-namespace", "N", "", "Only get application from namespace")
 	command.Flags().IntVar(&sourcePosition, "source-position", -1, "Position of the source from the list of sources of the app. Counting starts at 1.")
+	command.Flags().StringVar(&sourceName, "source-name", "", "Name of the source from the list of sources of the app.")


Same here, considering Name is not specific to a multi-source app, the description might need an update.

ishitasequeira · 2024-10-30T11:49:05Z

cmd/util/app.go

@@ -751,6 +753,8 @@ func ConstructSource(source *argoappv1.ApplicationSource, appOpts AppOptions, fl
 			setPluginOptEnvs(source, appOpts.pluginEnvs)
 		case "ref":
 			source.Ref = appOpts.ref
+		case "source-name":
+			source.Name = appOpts.SourceName


We should be checking if the name is already assigned to some other source within the app.spec.sources before allowing to set it.

I added the check in validateAndNormalizeApp to have a central validation for the both the UI and CLI API calls.

codecov · 2024-10-31T03:38:39Z

Codecov Report

Attention: Patch coverage is 0.64516% with 154 lines in your changes missing coverage. Please review.

Project coverage is 55.01%. Comparing base (0f872f5) to head (2c069d0).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/argocd/commands/app.go	0.00%	145 Missing ⚠️
server/application/application.go	0.00%	6 Missing and 1 partial ⚠️
cmd/util/app.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #20470      +/-   ##
==========================================
- Coverage   55.17%   55.01%   -0.16%     
==========================================
  Files         324      324              
  Lines       55256    55397     +141     
==========================================
- Hits        30487    30479       -8     
- Misses      22156    22300     +144     
- Partials     2613     2618       +5

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

ishitasequeira · 2024-11-04T13:05:38Z

pkg/apis/application/v1alpha1/types.go

@@ -191,6 +191,8 @@ type ApplicationSource struct {
 	Chart string `json:"chart,omitempty" protobuf:"bytes,12,opt,name=chart"`
 	// Ref is reference to another source within sources field. This field will not be used if used with a `source` tag.
 	Ref string `json:"ref,omitempty" protobuf:"bytes,13,opt,name=ref"`
+	// Name is used to refer to a source and is displayed in the UI.


Considering we want this to be of use only for a multi-source app, can we update the comment to reflect the same?

Sure thing. Updated.

ishitasequeira · 2024-11-04T13:06:20Z

cmd/argocd/commands/app.go

@@ -381,6 +396,19 @@ func NewApplicationGetCommand(clientOpts *argocdclient.ClientOptions) *cobra.Com
 			})
 			errors.CheckError(err)

+			if sourceName != "" && sourcePosition != -1 {


That works for me. Although, let's update the comment on the Name field in types.go to reflect that it is mainly used for multi-source apps.

ishitasequeira

The PR looks good!! Thanks @CefBoud!!

ishitasequeira · 2024-11-05T12:13:09Z

@agaudreault do you still have any concerns on the PR?

cmd/argocd/commands/app.go

andrii-korotkov-verkada · 2024-11-09T03:46:14Z

cmd/argocd/commands/app.go

+				if pos, ok := sourceNameToPosition[sourceName]; !ok {
+					log.Fatalf("Unknown source name '%s'", sourceName)
+				} else {
+					sourcePosition = int(pos)


Why convert to int instead of using int in getSourceNameToPositionMap?

int64 is already being used for sourcePosition in other places.

andrii-korotkov-verkada · 2024-11-09T03:47:13Z

cmd/argocd/commands/app.go

+				} else {
+					sourcePosition = int(pos)
+				}
+			}


Create a helper function for getting the position, since this code is copy-pasted between multiple commands.

The returned map is sometimes used in a loop rather than directly in an if statement. Its usage varies, especially when it’s reused within loops. Using a function would also require checking if the name exists, which means an additional if statement would be needed with the function. As a result, I'm not sure it would offer much more conciseness.

andrii-korotkov-verkada · 2024-11-09T03:47:32Z

cmd/argocd/commands/app.go

+				} else {
+					sourcePosition = int(pos)
+				}
+			}


Use a helper function.

andrii-korotkov-verkada · 2024-11-09T03:51:50Z

ui/src/app/applications/components/application-parameters/source-panel.tsx

@@ -204,6 +205,11 @@ export const SourcePanel = (props: {
                                                    </div>
                                                </div>
                                            </div>
+                                            <div className='row argo-form-row'>
+                                                <div className='columns small-10'>
+                                                    <FormField formApi={api} label='Name' field={'spec.source.name'} component={Text}></FormField>


Should we do an optional param checks, since source and name may not be present?

Their values are initialized with an empty string.

agaudreault

LGTM. Just missing tests on the errors cases for the new parameter

Signed-off-by: cef <[email protected]>

CefBoud · 2024-11-12T14:13:56Z

Thank you for the reviews. @agaudreault @ishitasequeira @andrii-korotkov-verkada

@agaudreault I added tests to check for param errors.

Signed-off-by: cef <[email protected]> Co-authored-by: Alexandre Gaudreault <[email protected]>

Signed-off-by: cef <[email protected]> Co-authored-by: Alexandre Gaudreault <[email protected]> Signed-off-by: Adrian Aneci <[email protected]>

CefBoud changed the title ~~feat(app): add Name to ApplicationSource~~ Add optional name reference to Source object Oct 20, 2024

CefBoud changed the title ~~Add optional name reference to Source object~~ Add optional 'name' to Source object Oct 20, 2024

CefBoud force-pushed the feat/add-name-multisource-apps branch from 220edee to b9e648b Compare October 20, 2024 17:21

CefBoud changed the title ~~Add optional 'name' to Source object~~ feat(app): Add optional 'name' to Source object Oct 20, 2024

CefBoud force-pushed the feat/add-name-multisource-apps branch from b9e648b to a45c12b Compare October 20, 2024 23:08

nitishfy reviewed Oct 21, 2024

View reviewed changes

CefBoud force-pushed the feat/add-name-multisource-apps branch 3 times, most recently from d70723f to f490b1d Compare October 21, 2024 23:12

CefBoud marked this pull request as ready for review October 22, 2024 00:18

CefBoud requested review from a team as code owners October 22, 2024 00:18

CefBoud force-pushed the feat/add-name-multisource-apps branch from f490b1d to e2650a1 Compare October 22, 2024 00:28

CefBoud marked this pull request as draft October 22, 2024 13:02

CefBoud force-pushed the feat/add-name-multisource-apps branch 3 times, most recently from bc501b0 to ed5b9b6 Compare October 23, 2024 00:49

nitishfy mentioned this pull request Oct 25, 2024

REQUEST: New membership for @nitishfy argoproj/argoproj#334

Closed

6 tasks

CefBoud marked this pull request as ready for review October 25, 2024 19:08

CefBoud force-pushed the feat/add-name-multisource-apps branch 3 times, most recently from 01ba671 to 39b0038 Compare October 26, 2024 21:42

CefBoud mentioned this pull request Oct 28, 2024

Add optional name reference to Source object #17731

Closed

agaudreault requested changes Oct 28, 2024

View reviewed changes

cmd/util/app.go Show resolved Hide resolved

test/e2e/app_multiple_sources_test.go Show resolved Hide resolved

ishitasequeira requested changes Oct 30, 2024

View reviewed changes

CefBoud force-pushed the feat/add-name-multisource-apps branch 2 times, most recently from 12252d8 to 0a9d9ac Compare October 31, 2024 00:38

CefBoud requested review from agaudreault and ishitasequeira October 31, 2024 01:00

CefBoud force-pushed the feat/add-name-multisource-apps branch from 0a9d9ac to 1376663 Compare October 31, 2024 01:45

ishitasequeira reviewed Nov 4, 2024

View reviewed changes

CefBoud force-pushed the feat/add-name-multisource-apps branch from 1376663 to 129a95b Compare November 4, 2024 13:34

CefBoud requested a review from ishitasequeira November 4, 2024 13:36

CefBoud force-pushed the feat/add-name-multisource-apps branch from 129a95b to b96f1db Compare November 4, 2024 14:35

ishitasequeira approved these changes Nov 5, 2024

View reviewed changes

andrii-korotkov-verkada reviewed Nov 9, 2024

View reviewed changes

agaudreault reviewed Nov 11, 2024

View reviewed changes

CefBoud force-pushed the feat/add-name-multisource-apps branch 2 times, most recently from 2301a0b to cad6a1a Compare November 12, 2024 14:06

feat(app): add Name to ApplicationSource

31c0cf8

Signed-off-by: cef <[email protected]>

CefBoud force-pushed the feat/add-name-multisource-apps branch from cad6a1a to 31c0cf8 Compare November 12, 2024 14:13

agaudreault approved these changes Nov 18, 2024

View reviewed changes

Merge branch 'master' into feat/add-name-multisource-apps

2c069d0

agaudreault enabled auto-merge (squash) November 18, 2024 20:21

agaudreault merged commit 83953fe into argoproj:master Nov 18, 2024
26 checks passed

dmosesson pushed a commit to dmosesson/argo-cd that referenced this pull request Nov 20, 2024

feat(app): Add optional 'name' to Source object (argoproj#20470)

edd7b67

Signed-off-by: cef <[email protected]> Co-authored-by: Alexandre Gaudreault <[email protected]>

andrii-korotkov-verkada mentioned this pull request Nov 21, 2024

App sync fails with 'failed to compare revisions for source 1 of 2' when new source is introduced first #20900

Open

3 tasks

CefBoud mentioned this pull request Nov 30, 2024

REQUEST: New membership for @CefBoud argoproj/argoproj#345

Open

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): Add optional 'name' to Source object #20470

feat(app): Add optional 'name' to Source object #20470

CefBoud commented Oct 20, 2024 •

edited

Loading

bunnyshell bot commented Oct 20, 2024 •

edited

Loading

nitishfy left a comment

agaudreault left a comment

ishitasequeira left a comment •

edited

Loading

ishitasequeira Oct 30, 2024

CefBoud Oct 30, 2024

ishitasequeira Nov 4, 2024

ishitasequeira Oct 30, 2024

ishitasequeira Oct 30, 2024

CefBoud Oct 31, 2024

codecov bot commented Oct 31, 2024 •

edited

Loading

ishitasequeira Nov 4, 2024

CefBoud Nov 4, 2024

ishitasequeira Nov 4, 2024

ishitasequeira left a comment

ishitasequeira commented Nov 5, 2024

andrii-korotkov-verkada Nov 9, 2024

CefBoud Nov 11, 2024 •

edited

Loading

andrii-korotkov-verkada Nov 9, 2024

CefBoud Nov 12, 2024

andrii-korotkov-verkada Nov 9, 2024

andrii-korotkov-verkada Nov 9, 2024

CefBoud Nov 12, 2024

agaudreault left a comment

CefBoud commented Nov 12, 2024

feat(app): Add optional 'name' to Source object #20470

feat(app): Add optional 'name' to Source object #20470

Conversation

CefBoud commented Oct 20, 2024 • edited Loading

Description

Screenshots

bunnyshell bot commented Oct 20, 2024 • edited Loading

❌ Preview Environment deleted from Bunnyshell

nitishfy left a comment

Choose a reason for hiding this comment

agaudreault left a comment

Choose a reason for hiding this comment

ishitasequeira left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Oct 31, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishitasequeira left a comment

Choose a reason for hiding this comment

ishitasequeira commented Nov 5, 2024

Choose a reason for hiding this comment

CefBoud Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agaudreault left a comment

Choose a reason for hiding this comment

CefBoud commented Nov 12, 2024

CefBoud commented Oct 20, 2024 •

edited

Loading

bunnyshell bot commented Oct 20, 2024 •

edited

Loading

ishitasequeira left a comment •

edited

Loading

codecov bot commented Oct 31, 2024 •

edited

Loading

CefBoud Nov 11, 2024 •

edited

Loading