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 deployment failed on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

🚀 /bns:deploy to redeploy the environment
❌ /bns:delete to remove 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.02%. Comparing base (16649c6) to head (b96f1db).
Report is 2 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.12%   55.02%   -0.11%     
==========================================
  Files         324      324              
  Lines       55197    55339     +142     
==========================================
+ Hits        30430    30452      +22     
- Misses      22145    22277     +132     
+ Partials     2622     2610      -12

☔ 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?

andrii-korotkov-verkada · 2024-11-09T03:45:12Z

cmd/argocd/commands/app.go

+	sourceNameToPosition := make(map[string]int64)
+	for i, s := range app.Spec.Sources {
+		if s.Name != "" {
+			sourceNameToPosition[s.Name] = int64(i + 1)


It's a bit confusing that we use indexes starting from 1, but this is out of scope of this PR.

@andrii-korotkov-verkada , We do not consider this to be an index, but the positions. There was a discussion in the community around this and we agreed to start with 1 on CLI and 0 on apis.

Postion = 1 --> Index = 0.

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.

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

Open

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

Open

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

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

ishitasequeira Nov 12, 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

Are you sure you want to change the base?

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 deployment failed on 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

Choose a reason for hiding this comment

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