Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add/Delete support in Sources tab for multi-source apps (#18047) #18804

Conversation

keithchong
Copy link
Contributor

Closes #18047

This change is based on top of the Edit support added from #17890
which in turn was based on the initial support that allows the viewing of multiple sources from #17275. This implements what is described in the proposal #17108.

Up to now, we have Edit support which matches what was available for single source apps (can edit and save parameter values). But this feature allows you to add and delete sources. Add was previously not available for single source apps, which would have converted a single source to a multi-source app. This feature does not allow you to do that conversion, but it allows the Add action to be performed on existing multi-source apps that use the sources field.

The new source-panel.tsx file has some similarities with the application-create-panel.tsx. I thought it better to not change the latter and keep things separated for now, if and when someone supports creating multi-source apps from the new app panel, then it will be good to common things up.

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).

@keithchong keithchong requested a review from a team as a code owner June 24, 2024 21:26
Copy link

bunnyshell bot commented Jun 24, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Copy link

bunnyshell bot commented Jun 24, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@saumeya
Copy link
Contributor

saumeya commented Jul 8, 2024

Hi Keith, this PR looks good to me, I tested it with a multi source application and the Add and Delete scenarios worked well.

While deleting the helm repo, I did get an unrelated error, see screenshot. I have looked in the code, it doesn'r seem to be coming from the delete api call. Don't know if it is related or not.

Screenshot 2024-07-08 at 1 08 02 PM

Also, currently if one source is remaining, the delete button is enabled, but it throws error while deleting, (understandable as we always want one source). Is it expected behaviour or should we disable the delete button when 1 source is remaining?

Copy link
Contributor

@saumeya saumeya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM , left 1 comment

@saumeya
Copy link
Contributor

saumeya commented Jul 8, 2024

Also, noticed a bug -
The floating title copies some attributes of the first resource,
like in the screenshot - the TYPE is correct but URL and Revision is not, it is copying from the first resource.

Screenshot 2024-07-08 at 2 18 28 PM

@keithchong keithchong requested review from a team as code owners August 21, 2024 12:43
@keithchong keithchong force-pushed the 18047-AddAndDeleteSupportForMultiSourceApps branch from 9d1a633 to 02671fd Compare August 21, 2024 12:57
@keithchong keithchong force-pushed the 18047-AddAndDeleteSupportForMultiSourceApps branch from 02671fd to 7d1dcc7 Compare August 21, 2024 18:29
@keithchong
Copy link
Contributor Author

Also, noticed a bug - The floating title copies some attributes of the first resource, like in the screenshot - the TYPE is correct but URL and Revision is not, it is copying from the first resource.

Screenshot 2024-07-08 at 2 18 28 PM

The fix is included in the following PR: #19623

@keithchong keithchong force-pushed the 18047-AddAndDeleteSupportForMultiSourceApps branch from 7d1dcc7 to d2761a0 Compare August 26, 2024 22:15
Signed-off-by: Keith Chong <kykchong@redhat.com>
@keithchong keithchong force-pushed the 18047-AddAndDeleteSupportForMultiSourceApps branch from d2761a0 to 25e753d Compare August 27, 2024 06:37
@keithchong
Copy link
Contributor Author

Delete button is disabled if one source remains:

Screenshot 2024-08-26 at 4 43 13 PM

@ashutosh16
Copy link
Contributor

ashutosh16 commented Aug 28, 2024

tested the basic functionality and looks good to me

Screenshot 2024-08-28 at 16 10 26

Signed-off-by: Keith Chong <kykchong@redhat.com>
@ashutosh16 ashutosh16 enabled auto-merge (squash) August 30, 2024 19:35
Copy link
Contributor

@ashutosh16 ashutosh16 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ishitasequeira
Copy link
Member

/bns:deploy

@ishitasequeira
Copy link
Member

/bns:start
/bns:deploy

@crenshaw-dev
Copy link
Member

/bns:start

auto-merge was automatically disabled September 4, 2024 14:12

Pull request was closed

@crenshaw-dev crenshaw-dev reopened this Sep 4, 2024
Copy link

bunnyshell bot commented Sep 4, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Copy link

bunnyshell bot commented Sep 4, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@crenshaw-dev
Copy link
Member

/bns:deploy

Copy link
Member

@ishitasequeira ishitasequeira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @saumeya @ashutosh16 for testing the PR and validating the code. I tested the PR as well and everything looks good.

@ishitasequeira ishitasequeira merged commit 6dc7405 into argoproj:master Sep 5, 2024
42 of 43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: Provide Add and Delete support in Sources tab for multi-source apps
5 participants