feat(application-controller): Add support for rollback multi-source applications #14124

JorTurFer · 2023-06-19T18:57:04Z

Signed-off-by: Jorge Turrado jorge.turrado@scrm.lidl

Currently, ArgoCD support multi-source applications and that's a really nice feature, but using multi source, end users have to sacrify the option of executing fast rollbacks because the rollbacks are not supported for multi-source applications.
This PR adds the support for rollback multi-source applications (as single source applications do)

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

Please see Contribution FAQs if you have questions about your pull-request.

Fixes #12580

codecov · 2023-06-22T21:46:22Z

Codecov Report

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

Project coverage is 49.90%. Comparing base (f87897c) to head (a1acb31).
Report is 43 commits behind head on master.

Files	Patch %	Lines
server/application/application.go	62.33%	22 Missing and 7 partials ⚠️
controller/sync.go	66.66%	3 Missing and 3 partials ⚠️
pkg/apis/application/v1alpha1/types.go	0.00%	4 Missing ⚠️
util/webhook/webhook.go	0.00%	4 Missing ⚠️
util/argo/ref_sources.go	90.90%	2 Missing and 1 partial ⚠️
cmd/argocd/commands/app.go	0.00%	0 Missing and 2 partials ⚠️
server/repository/repository.go	92.59%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #14124      +/-   ##
==========================================
+ Coverage   49.73%   49.90%   +0.17%     
==========================================
  Files         274      275       +1     
  Lines       48948    49331     +383     
==========================================
+ Hits        24343    24621     +278     
- Misses      22230    22310      +80     
- Partials     2375     2400      +25

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

JorTurFer · 2023-06-22T21:52:17Z

Please, put special attention reviewing the frontend because I'm not frontend developer and maybe I have done something terriblely wrong

JorTurFer · 2023-06-23T10:04:21Z

This is how it looks in the frontend:

ownyrd · 2023-07-03T11:33:20Z

Thanks for implementing this. Hope it gets merged soon :)

crenshaw-dev · 2023-07-18T18:34:33Z

Thanks for the PR! We're past feature freeze for 2.8, so I'll queue this up for the 2.9 roadmap.

JorTurFer · 2023-07-18T18:59:03Z

Thanks for the PR! We're past feature freeze for 2.8, so I'll queue this up for the 2.9 roadmap.

Sure 😄
Do you have any ETA for reviewing the PR? No rush at all, it's just for fixing merge conflicts when you are going to review it to not be rebasing it all the time xD

crenshaw-dev · 2023-07-18T19:03:09Z

Makes sense! At least 3 weeks out, I'm focusing on getting 2.8 ready for GA for now.

JorTurFer · 2023-07-18T19:09:07Z

Makes sense! At least 3 weeks out, I'm focusing on getting 2.8 ready for GA for now.

After vacations (I'll take them the first half of August so during second half) looks as a nice moment to rebase the PR with latest changes 😄
Thanks for the info 🙇

kewei5zhang · 2023-08-10T00:34:37Z

Hoping to have this merged soon. 🙏

JorTurFer · 2023-08-15T14:04:18Z

Hi @crenshaw-dev
Is it now a good moment to rebase my PR? 😄

crenshaw-dev

Only had time for a quick glance, opened one can of worms. :-)

reposerver/repository/repository.proto

JorTurFer · 2023-08-17T14:18:03Z

Sorry, I needed more time that I expected for rebasing the PR 🤦 (the summer break has erased my memory 100%)

JorTurFer · 2023-08-17T16:38:25Z

I have removed the logic from here and I have moved it into the state

Pehesi97 · 2023-08-22T19:16:22Z

Any news on this? Do you guys need an extra pair of hands?

JorTurFer · 2023-08-22T19:18:34Z

Any news on this? Do you guys need an extra pair of hands?

It's waiting a review 😄

crenshaw-dev · 2023-08-24T15:42:45Z

@Pehesi97 we need lots of extra pairs of hands. 😆

GuerreiroLeonardo · 2023-09-05T15:33:47Z

Any predictions on when it will be released?

JorTurFer · 2023-09-05T18:35:38Z

@GuerreiroLeonardo , if you could help them reviewing the code, I think this will be faster for sure 😄
I guess that we are a bit overloaded atm with the summer break and the last release

GuerreiroLeonardo · 2023-09-06T20:38:34Z

@GuerreiroLeonardo , if you could help them reviewing the code, I think this will be faster for sure 😄 I guess that we are a bit overloaded atm with the summer break and the last release

@JorTurFer JorTurFer I wish I could, I have no knowledge on the argoCD code. I am simply a user right now hahah. I gues it would take a lot of hours until I am proficient enough to give any relevant contribution. Sorry =/
I guess maybe crenshaw-dev could help

gioppoluca · 2023-09-08T12:00:36Z

We also consider this PR important, do not have to capabilities to look at the code, but could test it once it gets released.

jsoref

I'd want to see:

design doc
user facing documentation
a ui demo (so I can kick the tires) -- the picture in feat(application-controller): Add support for rollback multi-source applications #14124 (comment) should be in the description.

Note that I'm not wearing a project owner hat, this what I'd want as a drive-by reviewer / UI person / end user.

jsoref · 2023-09-26T17:19:43Z

util/argo/ref_sources.go

+}
+
+// GetRefSources creates a map of ref keys (from the sources' 'ref' fields) to information about the referenced source.
+// This function also validates the references use allowed characters and does not define the same ref key more than


Suggested change

// This function also validates the references use allowed characters and does not define the same ref key more than

// This function also validates the references use allowed characters and do not define the same ref key more than

Is a spelling error on a comment a good reason to ask for a review on such a feature? Seems a good way to make contributors stop contributing.

I took the code from argo.go and I moved it as it was and i didn't notice it. I'll update it 😄

jsoref · 2023-09-26T17:20:23Z

util/argo/ref_sources.go

+				}
+				refKey := "$" + source.Ref
+				if _, ok := refKeys[refKey]; ok {
+					return nil, fmt.Errorf("invalid sources: multiple sources had the same `ref` key")


it'd be really nice if errors like this reported at least a pair of sources so that users have something to chase

I took the code from argo.go and I moved it as it is but I could add the key to the message if you think is useful

My goal is to be able to read a message and figure out how to address it. If this error will be shown as is, I'd want to see the id of the thing (is it an application/<x>?) and two sources that had the same ref (preferably with the ref). If it's going to be pasted w/ other things, then as long as once it's pasted it has those bits, that'd make it usable.

I'd try to improve the message 😄
I'll review the logic trying to figure out the best way to print the message

jsoref · 2023-09-26T17:26:09Z

In:

Why do I see the same revision twice?

If I can't kick the tires, could you at please kick the tires for me?

User Story:

User has a "multi-source application" (some sample content for me to look at)
User deploys it (show picture of screens)
User makes a change (?) (show change, picture of how things change)
User needs to trigger a rollback (show path through UI that user takes to trigger it)
User sees rollback complete (show how the UI looks)
User wants to review to see what rollback(s) have happened (show path through UI for this).

JorTurFer · 2023-09-26T19:07:02Z

design doc
user facing documentation
a ui demo (so I can kick the tires) -- the picture in #14124 (comment) should be in the description.

Hi @jsoref
I appreciate your feedback but you couldn't see those things just because the PR template doesn't require them and I'm new contributing to this project.
I followed it and I have tried to do my best, but based on the documentation I had, those things weren't required. If you point me to any doc about how can I provide the UI demo, I'd be more than happy providing it. I'd really appreciate any guide on that part because I'm totally lost in front side.

About the user documentation, the UX is the same because I haven't added any new interaction, I just enabled an already existing button.

Why do I see the same revision twice?

Because probably I used the same repository for both sources, sorry. I didn't think that it was important.

If I can't kick the tires, could you at please kick the tires for me?

Except for how the multiple sources are presented, all the other points are exactly the same as the current scenario. It's true that I added the extra sources information to the UI (using the parenthesis), but I can remove it if you think that it's wrong.

ishitasequeira · 2024-04-11T00:28:28Z

util/argo/ref_sources.go

+// In case of rollback, this function also updates the targetRevision to the proper revision
+func GetRefSources(ctx context.Context, opts GetRefSourcesOptions) (argoappv1.RefTargetRevisionMapping, error) {
+	refSources := make(argoappv1.RefTargetRevisionMapping)
+	if len(opts.Sources) > 1 {


We can avoid the check len(opts.Sources) > 1 as the loop would not be executed if opts.Sources is empty.

Yeah, but you still can have 1 single item in sources slides, that's the reason. Originally this func received the app itself and checked if spec.HasMultipleSources() and this is the closer option.

I can remove it if you still think that it's not required

ishitasequeira

While testing the feature out, it seems, the application is not able to handle rollback from source to sources field or vice versa. To reproduce,

Create a single multi-source application
Update the application to be of a single source, i.e. remove spec.sources and add spec.source.
Try rolling back to the previous version. It does not rollback the app to spec.sources.

JorTurFer · 2024-04-14T15:25:52Z

While testing the feature out, it seems, the application is not able to handle rollback from source to sources field or vice versa. To reproduce,

yeah, I didn't support changing between both because I thought that it wasn't an scenario, but I can check and try to extend the PR to support it too, let me check it. Do the other changes work as expected?

…pplications Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

JorTurFer · 2024-04-14T22:20:11Z

@ishitasequeira , I've updated the PR supporting changes between single source and multi source. PTAL when you have some time.
I've also rebased my branch because there was a merge conflict related with a self generated file

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

JorTurFer · 2024-04-14T22:58:33Z

I'm not sure why the docker image is not built well, but I'd say that it's not related with my changes due to the message:

ERROR: failed to solve: process "/bin/sh -c apt-get update && apt-get install --no-install-recommends -y     openssh-server     nginx     unzip     fcgiwrap     git     git-lfs     make     wget     gcc     sudo     zip &&     apt-get clean &&     rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*" did not complete successfully: exit code: 100

keithchong · 2024-04-15T16:24:56Z

...pp/applications/components/application-deployment-history/application-deployment-history.tsx

-                                                }}
-                                                details={details}
-                                            />
+                                            <div className='row'>


I'm working on adding the multi-source support in what is currently the Parameters tab. For example, see PR. This is part of the proposal to support multi-source apps in the UI here, which references your PR. In order to better distinguish between the sources, I thought that showing the index within the sources field would help. So around this area, what do you think about adding a title here, something like what is depicted in my screenshots. It could be as simple as showing Source {i+1}

On a separate note, I made the current Edit card a collapsible section, and this is something 'new' to the UI. If there are many sources, and the list of parameters can get really long, it could get unwieldy and difficult to use. What you have here is fine as is, since the info is read-only. In order to get your PR merged, I think we could make these rows collapsible in a future PR (in order to align the multi-source presentation).

keithchong · 2024-04-15T18:41:52Z

ui/src/app/applications/components/application-operation-state/application-operation-state.tsx

+            value: (
+                <Revision
+                    repoUrl={utils.getAppDefaultSource(application).repoURL}
+                    revision={utils.getAppDefaultSyncRevision(application) + utils.getAppDefaultSyncRevisionExtra(application)}


Check if the extra part should be part of the revision field value, which might be included as part of the hyperlink

Just to follow up on this, the link is 'broken' because the 'and (1) more' phrase is included

keithchong · 2024-04-15T20:20:01Z

ui/src/app/applications/components/application-status-panel/application-status-panel.tsx

-                    <i className='fa fa-ellipsis-h' />
-                </button>
-            )}
+            {onClick && <button className='application-status-panel__more-button' />}


There was a 'More' button here before Ishita disabled it. Is this the new intended behavior?

keithchong · 2024-04-15T20:36:43Z

ui/src/app/applications/components/application-operation-state/application-operation-state.tsx

@@ -93,7 +93,15 @@ export const ApplicationOperationState: React.StatelessComponent<Props> = ({appl
        });
    }
    if (operationState.syncResult) {
-        operationAttributes.push({title: 'REVISION', value: <Revision repoUrl={utils.getAppDefaultSource(application).repoURL} revision={operationState.syncResult.revision} />});


Previously, the revision value wouldn't be displayed until the operation has finished. Seems like in this new behavior, the revision represents the most recent, successfully synced revision, and therefore will always show here, even if the operation is in progress. Is this the intended design? Is the if check on syncResult needed?

keithchong · 2024-04-16T14:30:02Z

ui/src/app/applications/components/application-status-panel/application-status-panel.tsx

@@ -107,13 +102,14 @@ export const ApplicationStatusPanel = ({application, showDiff, showOperation, sh
                    <div className='application-status-panel__item-name' style={{marginBottom: '0.5em'}}>
                        {application.spec.syncPolicy?.automated ? 'Auto sync is enabled.' : 'Auto sync is not enabled.'}
                    </div>
-                    {application.status && application.status.sync && application.status.sync.revision && !application.spec.source.chart && (
+                    {application.status && application.status.sync && application.status.sync.revision && revision && !application.spec.source.chart && (


Could application.status.sync.revision && revision be simplified to just revision?

keithchong · 2024-04-16T15:45:36Z

ui/src/app/applications/components/application-status-panel/application-status-panel.tsx

                        <div className='application-status-panel__item-name' style={{marginBottom: '0.5em'}}>
                            {appOperationState.phase} <Timestamp date={appOperationState.finishedAt || appOperationState.startedAt} />
                        </div>
-                        {(appOperationState.syncResult && appOperationState.syncResult.revision && (
+                        {(appOperationState.syncResult && revision && (


See my other comment regarding the operation sync result

ishitasequeira · 2024-04-18T02:04:54Z

I'm not sure why the docker image is not built well, but I'd say that it's not related with my changes due to the message:

ERROR: failed to solve: process "/bin/sh -c apt-get update && apt-get install --no-install-recommends -y     openssh-server     nginx     unzip     fcgiwrap     git     git-lfs     make     wget     gcc     sudo     zip &&     apt-get clean &&     rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*" did not complete successfully: exit code: 100

Thanks @JorTurFer for updating the PR with the use-case. It seems to be working now. The issue you mentioned has been fixed in master. Once you rebase your PR, the issue should get resolved.

GuerreiroLeonardo · 2024-04-25T13:14:19Z

Hi @JorTurFer any idea when you're gonna be able to look into these issues?

JorTurFer · 2024-04-28T10:26:24Z

I really appreciate the feedback @keithchong! Sadly, I don't have enough time nowadays to address it, there have been a lot of changes in the UI since I checked it time ago and I'm not frontend developer, so understand the UI requires a huge time to me. As the backend is almost ready, and it works, would be you willing to be invited to my repo and help me with those changes directly there (I've already invited you, just in case)? I feel that you have more context about how the things have to be done in front side. This offer is extensible to any other contributor who is willing to do it, I don't have problems adding them to my fork.

I see that you have pushed the UI proposal (btw, it looks nice, congrats!) and maybe you can adapt current UI to that proposal.

Other option can be just reverting all the UI changes and keep only the backend + cli support. Any other folk can continue from there on their own fork without all the (probably terrible) decisions that I took in front. @ishitasequeira ?

And sorry again, we are in the middle of a hard project and I don't think that I can solve the front issues soon 😞

keithchong · 2024-05-01T16:16:16Z

Hi @JorTurFer , reverting or taking away your UI changes might be a bit extreme. I'll see if I can fix the link issue and any other 'straight-forward' issues. For the other concerns, I think we should open follow-on Issues and address them in separate PRs, hopefully for the next release. Aligning the look and feel and adding back the 'More' button should not be blocking your PR. IMO, from the issues I saw, the incorrect status for the sync operation in progress is probably higher priority, because it is misleading information.

JorTurFer changed the title ~~enable buttons and support multiversion in details~~ feat(application-controller): Add support for rollback multi-source applications using the UI Jun 19, 2023

JorTurFer force-pushed the multi-source-history-rollback branch from fd30f30 to b2d284b Compare June 20, 2023 16:34

JorTurFer marked this pull request as ready for review June 22, 2023 16:29

JorTurFer marked this pull request as draft June 22, 2023 16:46

JorTurFer marked this pull request as ready for review June 22, 2023 21:45

JorTurFer changed the title ~~feat(application-controller): Add support for rollback multi-source applications using the UI~~ feat(application-controller): Add support for rollback multi-source applications Jun 30, 2023

crenshaw-dev requested changes Aug 15, 2023

View reviewed changes

reposerver/repository/repository.proto Outdated Show resolved Hide resolved

JorTurFer force-pushed the multi-source-history-rollback branch from fadc943 to 99e0b80 Compare August 17, 2023 14:16

jsoref reviewed Sep 26, 2023

View reviewed changes

ishitasequeira reviewed Apr 11, 2024

View reviewed changes

ishitasequeira reviewed Apr 14, 2024

View reviewed changes

JorTurFer and others added 12 commits April 14, 2024 23:07

feat(application-controller): Add support for rollback multi-source a…

465dc48

…pplications Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

regenerate codegen after rebase

a2335ef

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

fix tests

57de62e

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

fix front linting

6b5bcd5

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

update test

e1c14ae

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

update codegen

ab48e4a

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

Update server/application/application.go

cbec5f1

Co-authored-by: Ishita Sequeira <46771830+ishitasequeira@users.noreply.github.com> Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>

apply feedback

533a8b0

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

fix typo

b2af1f7

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

fix errors

aec91ff

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

add support for switching between single and multi

7c752b6

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

update codegen

6203cbf

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

JorTurFer force-pushed the multi-source-history-rollback branch from 867fbcc to 6203cbf Compare April 14, 2024 21:28

fix dereference issue

1f08ea0

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

remove unnecesary code

a1acb31

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>

keithchong reviewed Apr 15, 2024

View reviewed changes

keithchong reviewed Apr 16, 2024

View reviewed changes

	// This function also validates the references use allowed characters and does not define the same ref key more than
	// This function also validates the references use allowed characters and do not define the same ref key more than

feat(application-controller): Add support for rollback multi-source applications #14124

Are you sure you want to change the base?

feat(application-controller): Add support for rollback multi-source applications #14124

Conversation

JorTurFer commented Jun 19, 2023 • edited

codecov bot commented Jun 22, 2023 • edited

Codecov Report

JorTurFer commented Jun 22, 2023 • edited

JorTurFer commented Jun 23, 2023

ownyrd commented Jul 3, 2023 • edited

crenshaw-dev commented Jul 18, 2023

JorTurFer commented Jul 18, 2023

crenshaw-dev commented Jul 18, 2023

JorTurFer commented Jul 18, 2023 • edited

kewei5zhang commented Aug 10, 2023

JorTurFer commented Aug 15, 2023

crenshaw-dev left a comment

Choose a reason for hiding this comment

JorTurFer commented Aug 17, 2023

JorTurFer commented Aug 17, 2023

Pehesi97 commented Aug 22, 2023

JorTurFer commented Aug 22, 2023

crenshaw-dev commented Aug 24, 2023

GuerreiroLeonardo commented Sep 5, 2023

JorTurFer commented Sep 5, 2023

GuerreiroLeonardo commented Sep 6, 2023 • edited

gioppoluca commented Sep 8, 2023

jsoref left a comment • edited

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

Choose a reason for hiding this comment

jsoref commented Sep 26, 2023

JorTurFer commented Sep 26, 2023 • edited

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

JorTurFer commented Apr 14, 2024

JorTurFer commented Apr 14, 2024

JorTurFer commented Apr 14, 2024

keithchong Apr 15, 2024 • edited

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

ishitasequeira commented Apr 18, 2024

GuerreiroLeonardo commented Apr 25, 2024 • edited

JorTurFer commented Apr 28, 2024 • edited

keithchong commented May 1, 2024

JorTurFer commented Jun 19, 2023 •

edited

codecov bot commented Jun 22, 2023 •

edited

JorTurFer commented Jun 22, 2023 •

edited

ownyrd commented Jul 3, 2023 •

edited

JorTurFer commented Jul 18, 2023 •

edited

GuerreiroLeonardo commented Sep 6, 2023 •

edited

jsoref left a comment •

edited

JorTurFer commented Sep 26, 2023 •

edited

keithchong Apr 15, 2024 •

edited

GuerreiroLeonardo commented Apr 25, 2024 •

edited

JorTurFer commented Apr 28, 2024 •

edited