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

ACM-9431 - Auto import discovered ROSA clusters #3507

Merged

Conversation

Randy424
Copy link
Contributor

Regarding: https://issues.redhat.com/browse/ACM-9431

To test:

  1. In multicloud/credentials, setup OCM discovery credentials. You will need discoverable and importable ROSA and OCP clusters in your OCM environment.
  2. Go to the discovery tab (multicloud/infrastructure/clusters/discovered).
  3. Click the action menu (kebab menu) for your ROSA/OCP cluster and click Import cluster.
  4. For ROSA, import mode "Import from Red Hat OpenShift Cluster Manager" should be set, the cluster information should be pre-filled. Credential, ClusterID and Api token fields should be present.
  5. For OCP import mode "Enter your server URL and API token for the existing cluster" should be set.
  6. For Import cluster from the cluster list page (not from a discovered cluster), the import mode should be manual.

@Randy424
Copy link
Contributor Author

/hold

@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch from b1e11e4 to 9d76ddb Compare May 22, 2024 16:19
@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch 2 times, most recently from 134070a to cb9e127 Compare May 22, 2024 17:52
Copy link
Contributor

@KevinFCormier KevinFCormier left a comment

Choose a reason for hiding this comment

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

Awesome work! It works great. I left a few comments about code organization. There are also a couple functional issues we might want to address.

  1. Credential should include the namespace, as it does in the configuration screens. (See screenshots.)
  2. I think we should avoid passing the credential and the cluster ID to the form for non-ROSA clusters. If the user changes to use the RHOCM import mode and sees the pre-filled information, they might expect it to work.

image

image

@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch from cb9e127 to 9f62c15 Compare May 24, 2024 20:50
@Randy424
Copy link
Contributor Author

/retest

@Randy424
Copy link
Contributor Author

Currently making these changes: #3507 (review), will update thread when they're completed.

@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch from 9f62c15 to c1d36c5 Compare May 29, 2024 02:58
@Randy424
Copy link
Contributor Author

Randy424 commented May 29, 2024

Awesome work! It works great. I left a few comments about code organization. There are also a couple functional issues we might want to address.

  1. Credential should include the namespace, as it does in the configuration screens. (See screenshots.)
  2. I think we should avoid passing the credential and the cluster ID to the form for non-ROSA clusters. If the user changes to use the RHOCM import mode and sees the pre-filled information, they might expect it to work.

@KevinFCormier
I've made some changes to try and adopt the behavior of the discovery configuration wizard, but it looks like react-form-wizard does not support the kind of field validation I am trying to enforce. Namely, I'm not sure how I can prevent users from progressing in the wizard when the credential field is empty. The react-form-wizard exposes it's validation context to its proprietary components, and if I want to use a proprietary component it needs a path that corresponds to the template data. I am using fields that don't have a place within the template (won't have a path value) but still need to block the users progression when they are empty, as to mirror the validation of the other fields. It looks like the import mode field has this same issue, but we don't notice it because it always contains a value.

@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch 2 times, most recently from 8fe0510 to 1308ffa Compare May 29, 2024 21:16
Signed-off-by: Randy Bruno Piverger <rbrunopi@redhat.com>
@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch 2 times, most recently from 98fbfb4 to c428c1a Compare May 29, 2024 21:30
@Randy424
Copy link
Contributor Author

/retest

@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch from c428c1a to c5f60fa Compare May 29, 2024 22:01
@Randy424
Copy link
Contributor Author

@KevinFCormier This is ready for re-review! Thanks.

@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch 5 times, most recently from 1168add to ccbacaa Compare May 30, 2024 19:20
@Randy424
Copy link
Contributor Author

Randy424 commented May 30, 2024

…ard validation

Signed-off-by: Kevin Cormier <kcormier@redhat.com>
@Randy424 Randy424 force-pushed the ACM-9431-discovered-rosa-import branch from ccbacaa to 46a1602 Compare May 30, 2024 19:38
Copy link

sonarcloud bot commented May 30, 2024

@KevinFCormier
Copy link
Contributor

/lgtm

Copy link

openshift-ci bot commented May 30, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: KevinFCormier, Randy424

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [KevinFCormier,Randy424]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 13ef91b into stolostron:main May 30, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants