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

MGMT-15151: Refactoring external platform integrations control #2255

Conversation

ammont82
Copy link
Contributor

@ammont82 ammont82 commented Jul 10, 2023

Related to https://issues.redhat.com/browse/MGMT-15151

We refactor the partner integrations control, adding a dropdown in cluster details page and removing the switch for platform integration from Hosts discovery step.

  1. OCP version < 4.14 -> Oracle option disabled with tooltip + option='nutanix' selected
platform_integration_case1.mp4
  1. OCP version >= 4.14 -> Oracle option enabled with tooltip + option='vsphere' selected
platform_integration_case2.mp4
  1. OCP version >= 4.14 -> Oracle option enabled with tooltip + option='oci'(oracle) selected
platform_integration_case3.mp4

We disable 'Oracle' option when IBM/Z architecture was selected. It's related to https://issues.redhat.com/browse/MGMT-14991:
image

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 10, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 10, 2023

@ammont82: This pull request references MGMT-15151 which is a valid jira issue.

In response to this:

Related to https://issues.redhat.com/browse/MGMT-15151

We refactor the partner integrations control, adding a dropdown in cluster details page and removing the switch for platform integration from Hosts discovery step.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2023
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 11, 2023

@ammont82: This pull request references MGMT-15151 which is a valid jira issue.

In response to this:

Related to https://issues.redhat.com/browse/MGMT-15151

We refactor the partner integrations control, adding a dropdown in cluster details page and removing the switch for platform integration from Hosts discovery step.

external_integration_dropdown.mp4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ammont82 ammont82 marked this pull request as ready for review July 11, 2023 06:04
@ammont82 ammont82 requested a review from a team as a code owner July 11, 2023 06:04
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 11, 2023
@ammont82 ammont82 added the OCM label Jul 11, 2023
@ammont82 ammont82 force-pushed the MGMT-15151-refactoring-external-partners-integration-control branch from 7f78bc9 to 209fb0c Compare July 11, 2023 06:17
@ammont82 ammont82 force-pushed the MGMT-15151-refactoring-external-partners-integration-control branch from 209fb0c to dbb8f19 Compare July 11, 2023 06:23
Copy link
Contributor

@celdrake celdrake left a comment

Choose a reason for hiding this comment

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

I think there some extra clean-up to do / additional changes needed:

  1. Clean-up (there may be more, it's worth checking):
  • ExternalPartnerIntegrationsCheckbox is not used anymore
  • ExternalPartnerIntegrationsControl in Cypress tests + related tests should be adapted
  • externalPartnerIntegrations in OcmClusterDetailsValues should be removed too?
  • Do we still need useClusterSupportedPlatforms?
  1. Needs changes:

The Dropdown for the platform integration is editable after the cluster is created, but if it changes, the PATCH request does not contain the change, so changing it does not work.

  1. Open question:
    What's the impact in the Review & Create page? From what I see, the "platform-integration" only has Vsphere and Nutanix. Shouldn't all platforms be displayed there now?

@ammont82 ammont82 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 12, 2023
@ammont82 ammont82 added this to the v2.24 milestone Jul 12, 2023
@jkilzi
Copy link
Collaborator

jkilzi commented Jul 12, 2023

please rebase

@ammont82
Copy link
Contributor Author

ammont82 commented Jul 12, 2023

@celdrake :

  • Clean-up ->Done.

  • Needs changes:

The Dropdown for the platform integration is editable after the cluster is created, but if it changes, the PATCH request does not contain the change, so changing it does not work. -> True, I've need to change handleClusterUpdate method.

  • Open question:

What's the impact in the Review & Create page? From what I see, the "platform-integration" only has Vsphere and Nutanix. Shouldn't all platforms be displayed there now? -> Asked in JIRA ticket.

  • Tests: I have to adapt the tests. PENDING

@ammont82 ammont82 force-pushed the MGMT-15151-refactoring-external-partners-integration-control branch from ca26015 to 4450e26 Compare July 13, 2023 07:19
Copy link
Collaborator

@jgyselov jgyselov left a comment

Choose a reason for hiding this comment

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

Needs tests: at the very least to verify that the requests contain the correct values and that the options (especially) are correctly enabled or disabled. Both when creating and updating the cluster.

As for the Review & create page impact, we currently only show that some platform integration has been selected (without any specifics) and the "You will need to modify your platform configuration after cluster installation is completed." note. So unless we want to be specific, there are no changes needed. That's a UX question, though.

@ammont82
Copy link
Contributor Author

Needs tests: at the very least to verify that the requests contain the correct values and that the options (especially) are correctly enabled or disabled. Both when creating and updating the cluster.

As for the Review & create page impact, we currently only show that some platform integration has been selected (without any specifics) and the "You will need to modify your platform configuration after cluster installation is completed." note. So unless we want to be specific, there are no changes needed. That's a UX question, though.

I'm working on the tests.
I think for Review&Create we don't need changes.

@nirfarkas
Copy link
Contributor

nirfarkas commented Jul 13, 2023

For the disabling question:
Can we disable the whole drop-down if the user selected Oracle, clicked "Next" and then "Back"?
While keeping the drop-down "alive" if they select any other option?

A few nits:
No need for "Learn more" for the "None" option.
image

The disabled tooltip (OCP ver.)should contain the message: "Integration with Oracle is available for OpenShift 4.14 and later versions."
The tooltip is a bit far away up and visually detached from the Oracle row.
image

@ammont82
Copy link
Contributor Author

ammont82 commented Jul 13, 2023

@nirfarkas :

  • For the disabling question:
    Can we disable the whole drop-down if the user selected Oracle, clicked "Next" and then "Back"?
    While keeping the drop-down "alive" if they select any other option?
    Yes, we can.

  • No "Learn more" link in None option:
    image

  • About tooltip, I don't know if I can move. I'll take a look to the component:
    image

@celdrake
Copy link
Contributor

Needs tests: at the very least to verify that the requests contain the correct values and that the options (especially) are correctly enabled or disabled. Both when creating and updating the cluster.

As for the Review & create page impact, we currently only show that some platform integration has been selected (without any specifics) and the "You will need to modify your platform configuration after cluster installation is completed." note. So unless we want to be specific, there are no changes needed. That's a UX question, though.

What I meant about R&C,is that the note is only for Nutanix and Vsphere. Is it correct not to show anything about OCI in the case it would be activated for a particular cluster?

@ammont82 ammont82 requested a review from celdrake July 24, 2023 13:59
@ammont82 ammont82 force-pushed the MGMT-15151-refactoring-external-partners-integration-control branch from d860c95 to db82922 Compare July 24, 2023 14:04
@ammont82 ammont82 force-pushed the MGMT-15151-refactoring-external-partners-integration-control branch from db82922 to 977dc9a Compare July 24, 2023 14:08
@openshift-ci-robot
Copy link

openshift-ci-robot commented Jul 25, 2023

@ammont82: This pull request references MGMT-15151 which is a valid jira issue.

In response to this:

Related to https://issues.redhat.com/browse/MGMT-15151

We refactor the partner integrations control, adding a dropdown in cluster details page and removing the switch for platform integration from Hosts discovery step.

  1. OCP version < 4.14 -> Oracle option disabled with tooltip + option='nutanix' selected
platform_integration_case1.mp4
  1. OCP version >= 4.14 -> Oracle option enabled with tooltip + option='vsphere' selected
platform_integration_case2.mp4
  1. OCP version >= 4.14 -> Oracle option enabled with tooltip + option='oci'(oracle) selected
platform_integration_case3.mp4

We disable 'Oracle' option when IBM/Z architecture was selected. It's related to https://issues.redhat.com/browse/MGMT-14991:
image

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jul 25, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ammont82, celdrake, jkilzi

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

Needs approval from an approver in each of these files:

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

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 25, 2023
@openshift-ci
Copy link

openshift-ci bot commented Jul 25, 2023

New changes are detected. LGTM label has been removed.

@ammont82 ammont82 merged commit 672bc15 into openshift-assisted:master Jul 25, 2023
8 checks passed
@jkilzi jkilzi modified the milestone: v2.24 Sep 13, 2023
rawagner pushed a commit to rawagner/facet-lib that referenced this pull request Sep 13, 2023
…hift-assisted#2255)

* Refactoring external partner integrations control

* Disabling Oci option in dropdown

* Removing ExternalPartnerIntegrationsCheckbox and adjusting code

* Change handleClusterUpdate to take into account changes in platform

* Renaming props in ExternalPlatformDropdown

* Cleaning code

* Adding tests form external platform integration dropdown

* Removing unnecessary tests

* Removing ASSISTED_INSTALLER_PLATFORM_INTEGRATION_FEATURE feature

* Disable dropdown when oci is selected as platform

* Adjusting texts for new component

* Change in texts and in ExternalPlatformDropdown

* Removing console.logs

* Remove ClusterPlatformIntegrationHint from footer

* Removing setPlatform method from HostDiscoveryService

* Disabling custom manifests checkbox when oci option is selected

* Change platform accordly with userManagedNetworking activated or not

* Solving lint problems

* Cleaning code and changing 'None' for 'No platform integration'

* Disabling UMN for vSphere option

* Adapting tests for external partner integration

* Solving problems with translation files

* Adapting changes from master

* Replacing link for button in external partner integration dropdown

* Set clusterPlatform as value of dropdown if exists

* Change resetPlatform to correct values

* Adapting tests for external partner integrations

* Patch correct platform type

* Cleaning code

* Disable Oracle option when is not supported by some cpu architecture

---------

Co-authored-by: Jonathan Kilzi <[email protected]>
@ammont82 ammont82 deleted the MGMT-15151-refactoring-external-partners-integration-control branch December 21, 2023 07:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. OCM size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants