feat: Support cluster name on Application destination. Closes #1548 #2808

lcostea · 2019-12-04T18:51:32Z

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).
I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
Optional. My organization is added to the README.
I've signed the CLA and my build is green (troubleshooting builds).

codecov · 2019-12-04T19:10:39Z

Codecov Report

Merging #2808 into master will increase coverage by 0.12%.
The diff coverage is 52.30%.

@@            Coverage Diff             @@
##           master    #2808      +/-   ##
==========================================
+ Coverage   41.70%   41.83%   +0.12%     
==========================================
  Files         116      116              
  Lines       16744    16726      -18     
==========================================
+ Hits         6983     6997      +14     
+ Misses       8855     8828      -27     
+ Partials      906      901       -5

Impacted Files	Coverage Δ
pkg/apis/application/v1alpha1/types.go	`60.97% <0.00%> (-0.91%)`	⬇️
server/application/application.go	`28.31% <0.00%> (-0.31%)`	⬇️
cmd/argocd/commands/app.go	`5.62% <33.33%> (+0.05%)`	⬆️
controller/appcontroller.go	`43.78% <57.14%> (+0.11%)`	⬆️
util/argo/argo.go	`74.40% <87.87%> (+1.60%)`	⬆️
server/rbacpolicy/rbacpolicy.go	`82.43% <0.00%> (+3.55%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9633417...cb9c834. Read the comment docs.

lcostea · 2019-12-06T20:01:19Z

@alexec When you have some time, please take a look at this.
I tried to follow the ideas discussed on #2381 to switch asap to use the Server field, so this way there will be very few changes to the code.

I still need to check/update the UI, but for that I will open another PR if needed

lcostea · 2019-12-19T20:45:10Z

Any chance to get some feedback now before the holidays, so I will have time to address it?

alexmt · 2019-12-19T21:11:24Z

Sorry for the delay with review @lcostea . We've got distracted by v1.3 performance challenges. Definitely looking at the PR today evening or first thing tomorrow morning

util/argo/argo.go

alexmt · 2019-12-20T23:21:59Z

controller/appcontroller.go

@@ -916,6 +916,10 @@ func (ctrl *ApplicationController) refreshAppConditions(app *appv1.Application)
 			})
 		}
 	} else {
+		destCondition := argo.ValidateDestination(context.Background(), &app.Spec.Destination, ctrl.db)


We need to infer server field in processAppOperationQueueItem method too. E.g. finalizeApplicationDeletion uses Destination.Server field.

util/argo/argo.go

details ----------- - dev env controller to provision a gke cluster through crossplane and install an argocd application - uses kubebuilder - based off an experimental PR in argocd: argoproj/argo-cd#2808 - provisions nodepools through controller now. This should be handled by crossplane in the future

vadasambar · 2020-01-10T10:10:21Z

Any updates on this?
I think this will open up the doors for really interesting things. I am eagerly looking forward to this PR getting merged.

lcostea · 2020-01-13T19:17:31Z

I had a little longer vacation, but I am back on this and hoping to get it done in the following days.

server/application/application_test.go

CLAassistant · 2020-03-17T12:53:29Z

All committers have signed the CLA.

vadasambar · 2020-04-09T10:04:05Z

Hey @lcostea, any updates on this PR?

shaikatz · 2020-04-23T08:40:10Z

Eager to see that merged.

lcostea · 2020-04-23T18:32:36Z

Working now to have this rebased, to fix the failing tests and increase code coverage
Hope to be ready for a review in 1-2 weeks

micke · 2020-04-23T19:04:53Z

@lcostea really appreciate the work you are doing!

vadasambar · 2020-04-24T05:46:37Z

Thank you @lcostea !

lcostea · 2020-05-04T06:41:48Z

Now that I reached a good test coverage, the next couple of days I will concentrate on doing many manual tests with 1-2 external clusters addressed by name in the apps

alexmt · 2020-06-03T22:13:32Z

Thank you for resolving merge conflicts!

cyrus-mc · 2020-06-14T18:34:54Z

+1 for this feature.

Would make it easier on Developers who usually only know the cluster by name, not server address.

…nation server URL

alexmt · 2020-06-19T18:16:31Z

@lcostea , thanks a lot for PR and patience!

My only concern is the isServerInferred field in CRD and Argo CD API . This is internal field and should not be visible. I found that is can be removed if field annotation is changed to json:"-" . I've implemented changes trying to find solution and send PR to your branch: https://github.com/lcostea/argo-cd/pull/1/files

Let me know if it looks good, please.

Hide internal field `isServerInferred`

lcostea · 2020-06-20T16:34:07Z

@alexmt I integrated your changes, but I can't get it to work with the

isServerInferred bool `json:"-"`

I tried a couple of options, but either fails on lint or on crd generation

alexmt

LGTM!

micke · 2020-06-20T23:37:30Z

Great work guys! This is going to make declarative setup so much better!

In which release will this change be included? Next major or sooner?

mgoodness · 2020-06-21T00:58:18Z

This is a fantastic improvement in useability. Thanks, @lcostea!

lcostea · 2020-06-21T09:28:30Z

thanks @alexmt for your support.
Next I am going to look at the UI, it could use some improvements when using cluster name,

cyrus-mc · 2020-07-08T19:19:52Z

@lcostea I have been running a build with this change in a non-prod environment.

From time to time an error is shown on the application page with the message "NotFound desc = cluster "" not found".

This sometimes results in sync or delete of application to hang with the following message in the application-controller logs

level=info msg="Unable to delete application resources: rpc error: code = NotFound desc = cluster \"\" not found" application=argocd-helloworld-test dest-namespace=test dest-server= reason=StatusRefreshed type=Warning

lcostea · 2020-07-09T18:25:52Z

@cyrus-mc thanks for letting me know, I am checking this. The sync and delete, are you doing them from the UI?

cyrus-mc · 2020-07-13T14:34:48Z

@cyrus-mc thanks for letting me know, I am checking this. The sync and delete, are you doing them from the UI?

Yes. It is odd because it doesn't always happen. But I do see the error pretty much all the time. Just that sync or delete aren't impacted.

I am using EKS with IAM authentication.

lcostea · 2020-07-15T13:35:17Z

@cyrus-mc I managed to reproduce it. I just opened a PR for changes in the UI part to support destination name (#3944) and next I am checking this issue.

alexmt · 2020-08-03T22:39:21Z

@lcostea , @cyrus-mc FYI. I've also reproduced cluster \"\" not found" error. Apparently we've forgotten to add argo.ValidateDestination in two places. Prepared PR with the fix: #4039

alexec self-assigned this Dec 6, 2019

alexec self-requested a review December 6, 2019 22:43

alexmt mentioned this pull request Dec 11, 2019

argocd cluster get throws error #2860

Closed

lcostea changed the title ~~Support cluster name on Application destination. Closes #1548~~ feat: Support cluster name on Application destination. Closes #1548 Dec 19, 2019

alexmt assigned alexmt and unassigned alexec Dec 20, 2019

alexmt reviewed Dec 20, 2019

View reviewed changes

util/argo/argo.go Show resolved Hide resolved

golangcibot reviewed Jan 16, 2020

View reviewed changes

server/application/application_test.go Outdated Show resolved Hide resolved

lcostea added 5 commits June 17, 2020 22:15

Support cluster name on Application destination

dc04606

fix(unique cluster): Validate only one cluster matches the name

5aad847

Fail on both destination name and server

a640ef7

fix(infer server): Calculate server field on app operations queue

2c9754f

Stop saving server field when it is infered

6b8a48a

lcostea and others added 5 commits June 17, 2020 22:18

Specify app destination supports only one of server and name

ab73b01

Test validate destination

bf23635

Allow destination name and server when inferred

8aabcfe

don't serialize internal field IsServerInferred field

cdb5c4d

ensure application server validate app destination before using desti…

38de1ce

…nation server URL

lcostea added 3 commits June 20, 2020 12:02

Merge pull request #1 from alexmt/alex/add_destination_name_alias

8c0f7c0

Hide internal field `isServerInferred`

Remove unnecessary json tag for unexported field

1059f47

Restore json tag

3afe88c

disable isServerInferred field linting

cb9c834

alexmt approved these changes Jun 20, 2020

View reviewed changes

alexmt merged commit 7ccb16b into argoproj:master Jun 20, 2020

lcostea deleted the liviu/add_destination_name_alias branch June 21, 2020 09:28

lcostea mentioned this pull request Jul 15, 2020

feat: Support applications with cluster name in the ui #1548 #3944

Merged

5 tasks

alexmt mentioned this pull request Aug 3, 2020

fix: infer app destination server URL prior app deletion and during lightweight reconciliation #4039

Merged

sivchari mentioned this pull request Aug 19, 2024

Panic due to un-exported field #19583

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Support cluster name on Application destination. Closes #1548 #2808

feat: Support cluster name on Application destination. Closes #1548 #2808

lcostea commented Dec 4, 2019

codecov bot commented Dec 4, 2019 •

edited

Loading

lcostea commented Dec 6, 2019

lcostea commented Dec 19, 2019

alexmt commented Dec 19, 2019

alexmt Dec 20, 2019

vadasambar commented Jan 10, 2020

lcostea commented Jan 13, 2020

CLAassistant commented Mar 17, 2020

vadasambar commented Apr 9, 2020

shaikatz commented Apr 23, 2020

lcostea commented Apr 23, 2020

micke commented Apr 23, 2020

vadasambar commented Apr 24, 2020

lcostea commented May 4, 2020

alexmt commented Jun 3, 2020

cyrus-mc commented Jun 14, 2020

alexmt commented Jun 19, 2020

lcostea commented Jun 20, 2020

alexmt left a comment

micke commented Jun 20, 2020

mgoodness commented Jun 21, 2020

lcostea commented Jun 21, 2020

cyrus-mc commented Jul 8, 2020

lcostea commented Jul 9, 2020

cyrus-mc commented Jul 13, 2020

lcostea commented Jul 15, 2020

alexmt commented Aug 3, 2020

feat: Support cluster name on Application destination. Closes #1548 #2808

feat: Support cluster name on Application destination. Closes #1548 #2808

Conversation

lcostea commented Dec 4, 2019

codecov bot commented Dec 4, 2019 • edited Loading

Codecov Report

lcostea commented Dec 6, 2019

lcostea commented Dec 19, 2019

alexmt commented Dec 19, 2019

alexmt Dec 20, 2019

Choose a reason for hiding this comment

vadasambar commented Jan 10, 2020

lcostea commented Jan 13, 2020

CLAassistant commented Mar 17, 2020

vadasambar commented Apr 9, 2020

shaikatz commented Apr 23, 2020

lcostea commented Apr 23, 2020

micke commented Apr 23, 2020

vadasambar commented Apr 24, 2020

lcostea commented May 4, 2020

alexmt commented Jun 3, 2020

cyrus-mc commented Jun 14, 2020

alexmt commented Jun 19, 2020

lcostea commented Jun 20, 2020

alexmt left a comment

Choose a reason for hiding this comment

micke commented Jun 20, 2020

mgoodness commented Jun 21, 2020

lcostea commented Jun 21, 2020

cyrus-mc commented Jul 8, 2020

lcostea commented Jul 9, 2020

cyrus-mc commented Jul 13, 2020

lcostea commented Jul 15, 2020

alexmt commented Aug 3, 2020

codecov bot commented Dec 4, 2019 •

edited

Loading