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

fix: UI: Nothing happens selecting cluster URL/Name dropdown (#13655) #21028

Conversation

keithchong
Copy link
Contributor

@keithchong keithchong commented Dec 3, 2024

Fixes #13655

There are actually multiple issues that this PR will try to address.

  • Need to sync (or 'match') changes made in the YAML Editor with the Form (As mentioned in the issue above, one PR attempted to fix this, but it resulted in this problem)
  • We need a way to distinguish between changes made from the form, and changes made from the YAML editor form
  • When changing the Destination type between URL and NAME, the URL field or Name field will get cleared out. The problem with the existing code is that attempts to delete these right away on the app, but it’s not working properly, because on re-render, it is reset

There are several test scenarios affected by this.

  • Make a change in the destination type combo
  • Make a change in the destination field
  • Go to the Yaml Editor, verify the change;
  • Make changes in the Yaml Editor and Save them, and verify in the Form view that the Destination field and type combo are correct
  • In the Yaml Editor, use empty string values for server and name
  • In the Yaml Editor, use non-empty string values for server and name
  • In the Yaml Editor, add both server and name
  • Any combinations of the above
  • Although the changes in this PR are 'small', one change may just break one of these scenarios. (I've reduced it down this current logic)

Note:

  • One can argue that server and name in the interface ApplicationDestination in models.ts should be made optional, but for the purposes of this PR, I just changed the initial value to be undefined, and then later on in the logic, one of these would be set to the empty string. (This explains that code change).

Previous Issues and PRs related to this.

#13813
#14216

and

#11228
#12054

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 December 3, 2024 01:12
Copy link

bunnyshell bot commented Dec 3, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@keithchong keithchong force-pushed the 13655-DestinationClusterURLAndNameIssue branch from 028274e to afa31f2 Compare December 3, 2024 13:25
@aali309
Copy link
Contributor

aali309 commented Dec 3, 2024

LGTM!

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.

LGTM!!

@ishitasequeira ishitasequeira merged commit 522d07a into argoproj:master Dec 4, 2024
30 checks passed
adriananeci pushed a commit to adriananeci/argo-cd that referenced this pull request Dec 4, 2024
…j#13655) (argoproj#21028)

Signed-off-by: Keith Chong <kykchong@redhat.com>
Signed-off-by: Adrian Aneci <aneci@adobe.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Nothing happens select cluster name option in Destination Tab when creating an application.
4 participants