[master] MGMT-20727: Cisco Intersight URL should be configurable#3332
Conversation
|
@openshift-cherrypick-robot: Ignoring requests to cherry-pick non-bug issues: MGMT-20727 DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR refactors the CIM configuration modal to use Formik for form state management instead of direct React state, while introducing Cisco Intersight URL configuration support. Changes include replacing the monolithic CimConfigurationForm with a Formik-based CimConfigurationFormFields component, updating persist.ts to use dynamic-plugin-sdk k8s operations instead of passed-in helper functions, adding localization entries for Cisco Intersight validation, and defining new K8sModel exports. The old resources.ts type definitions are removed in favor of SDK-based models. Changes
Sequence DiagramsequenceDiagram
actor User
participant Modal as CimConfigurationModal
participant Formik
participant Form as CimConfigurationFormFields
participant K8s as K8s SDK Operations
participant Cluster as OpenShift Cluster
User->>Modal: Open CIM Configuration
Modal->>K8s: Load AgentServiceConfig
K8s->>Cluster: k8sGet AgentServiceConfig
Cluster-->>K8s: Resource data + Cisco URL annotation
K8s-->>Modal: Resource loaded
Modal->>Form: Initialize Formik with values
Note over Formik: Storage sizes, LoadBalancer config,<br/>Cisco Intersight URL fields
Form->>User: Display form with Cisco URL option
User->>Form: Fill form & enable Cisco URL
Form->>Formik: Update values (addCiscoIntersightUrl=true)
Formik->>Formik: Validate schema (URL format, sizes)
alt Validation Success
Formik->>Form: Valid state
Form->>User: Enable Submit button
User->>Modal: Click Submit
Modal->>K8s: onEnableCIM(values)
rect rgb(200, 220, 255)
Note over K8s: Create/Patch resources with Cisco URL
K8s->>Cluster: k8sCreate IngressController
K8s->>Cluster: k8sPatch Route + Provisioning
K8s->>Cluster: k8sCreate AgentServiceConfig<br/>(with ciscoIntersightURL annotation)
end
Cluster-->>K8s: Resources created/updated
K8s-->>Modal: Success
Modal->>User: Close modal
else Validation Error
Formik->>Form: Invalid state with error messages
Form->>User: Display error indicators
User->>Form: Correct input
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-ci
bot
added
the
size/XXL
openshift-ci-robot
added
the
jira/valid-reference
|
@openshift-cherrypick-robot: This pull request references MGMT-20727 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
libs/ui-lib/lib/common/config/docs_links.ts (1)
150-155: URL-encode thedownloadIsoUrlquery parameter for special character safety.The
ciscoUrlparameter is validated at the form level in CimConfigurationModal.tsx usingparseUrl()to ensure it starts withhttp://orhttps://, and this validation occurs before persistence to the Kubernetes annotation, so that concern is already addressed.However, the
downloadIsoUrlparameter should be URL-encoded in the query string to safely handle special characters:return `${ciscoUrl}?_workflow_Version=1&IsoUrl=${encodeURIComponent(downloadIsoUrl)}`;Apply this to both branches of the function.
libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationFormFields.tsx (1)
39-58: Consider adding missing dependencies or documenting the rationale.The
useEffectdisablesreact-hooks/exhaustive-depsbut omitsisEdit,setFieldValue, andsetConfigureLoadBalancerInitialfrom the dependency array. WhilesetFieldValuefrom Formik is typically stable, the others could potentially cause stale closure issues if they change.If these are intentionally omitted because:
isEditis constant for the component's lifetimesetConfigureLoadBalancerInitialis a stable setterConsider adding a brief comment explaining why, or include them in the dependency array to be safe.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
libs/locales/lib/en/translation.json(3 hunks)libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx(5 hunks)libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationForm.css(0 hunks)libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationForm.tsx(0 hunks)libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationFormFields.tsx(1 hunks)libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.css(0 hunks)libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsx(2 hunks)libs/ui-lib/lib/cim/components/modals/CimConfiguration/persist.ts(14 hunks)libs/ui-lib/lib/cim/components/modals/CimConfiguration/types.ts(1 hunks)libs/ui-lib/lib/cim/hooks/index.tsx(1 hunks)libs/ui-lib/lib/cim/hooks/useAgentServiceConfig.tsx(1 hunks)libs/ui-lib/lib/cim/types/index.ts(0 hunks)libs/ui-lib/lib/cim/types/models.tsx(1 hunks)libs/ui-lib/lib/cim/types/resources.ts(0 hunks)libs/ui-lib/lib/common/components/clusterConfiguration/DownloadIso.tsx(2 hunks)libs/ui-lib/lib/common/components/ui/formik/InputField.tsx(2 hunks)libs/ui-lib/lib/common/components/ui/formik/types.ts(2 hunks)libs/ui-lib/lib/common/config/docs_links.ts(1 hunks)
💤 Files with no reviewable changes (5)
- libs/ui-lib/lib/cim/types/index.ts
- libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationForm.css
- libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.css
- libs/ui-lib/lib/cim/types/resources.ts
- libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationForm.tsx
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
📚 Learning: 2025-10-19T17:22:52.502Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/networkConfigurationValidation.ts:69-69
Timestamp: 2025-10-19T17:22:52.502Z
Learning: CIM UI changes in the repository openshift-assisted/assisted-installer-ui (e.g., files under libs/ui-lib/lib/cim/) are handled separately by the CIM team and should be tracked via separate issues rather than being included in PRs for other UI components.
Applied to files:
libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationFormFields.tsxlibs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsxlibs/ui-lib/lib/cim/components/modals/CimConfiguration/persist.ts
📚 Learning: 2025-10-21T04:40:36.292Z
Learnt from: linoyaslan
Repo: openshift-assisted/assisted-installer-ui PR: 3190
File: libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx:55-63
Timestamp: 2025-10-21T04:40:36.292Z
Learning: In libs/ui-lib/lib/ocm/components/clusterConfiguration/networkConfiguration/AdvancedNetworkFields.tsx, the network reordering logic in the useEffect (swapping clusterNetworks and serviceNetworks based on the primary machine network's IP family) is for UI consistency only. Validation of empty or invalid CIDRs is handled separately by validation schemas, not by the reordering logic.
Applied to files:
libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationFormFields.tsx
📚 Learning: 2025-08-11T06:07:38.056Z
Learnt from: ammont82
Repo: openshift-assisted/assisted-installer-ui PR: 3101
File: libs/ui-lib/lib/common/config/docs_links.ts:203-221
Timestamp: 2025-08-11T06:07:38.056Z
Learning: The MetalLB, OADP, Cluster Observability, and NUMA Resources operators in the virtualization bundle don't require version fallback logic in their documentation link functions (getMetalLbLink, getOadpLink, getClusterObservabilityLink, getNumaResourcesLink) in libs/ui-lib/lib/common/config/docs_links.ts, unlike some other operators like LSO and NVIDIA GPU.
Applied to files:
libs/ui-lib/lib/common/config/docs_links.ts
📚 Learning: 2025-12-17T09:08:07.992Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/cim/components/helpers/toAssisted.ts:199-201
Timestamp: 2025-12-17T09:08:07.992Z
Learning: In libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, the platformType field on agentClusterInstall.spec is guaranteed to always contain a valid PlatformType value (when lowercased), making the type assertion safe without additional runtime validation.
Applied to files:
libs/ui-lib/lib/cim/types/models.tsx
🧬 Code graph analysis (4)
libs/ui-lib/lib/cim/hooks/useAgentServiceConfig.tsx (3)
libs/ui-lib/lib/cim/hooks/types.tsx (1)
K8sWatchHookProps(8-8)libs/ui-lib/lib/cim/hooks/useK8sWatchResource.tsx (1)
useK8sWatchResource(13-27)libs/ui-lib/lib/cim/types/k8s/agent-service-config.ts (1)
AgentServiceConfigK8sResource(23-33)
libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationFormFields.tsx (3)
libs/ui-lib/lib/cim/components/modals/CimConfiguration/types.ts (2)
CimConfigurationFormFieldsProps(17-25)CimConfigurationValues(3-10)libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/cim/components/modals/CimConfiguration/persist.ts (1)
isIngressController(134-146)
libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx (3)
libs/ui-lib/lib/common/hooks/index.ts (1)
useTranslation(3-3)libs/ui-lib/lib/cim/hooks/useAgentServiceConfig.tsx (1)
useAgentServiceConfig(5-15)libs/ui-lib/lib/cim/types/k8s/infra-env-k8s-resource.ts (1)
InfraEnvK8sResource(8-42)
libs/ui-lib/lib/cim/components/modals/CimConfiguration/persist.ts (1)
libs/ui-lib/lib/cim/types/models.tsx (4)
RouteModel(36-45)IngressControllerModel(25-34)ProvisioningModel(47-56)AgentServiceConfigModel(14-23)
🪛 Biome (2.1.2)
libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsx
[error] 105-105: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (24)
libs/ui-lib/lib/common/components/ui/formik/types.ts (1)
8-8: LGTM: Type refinement aligns with PatternFly API.The change from
TextInputTypestoTextInputProps['type']improves type accuracy by directly referencing the PatternFly library's type definition.Also applies to: 67-67
libs/locales/lib/en/translation.json (1)
161-163: LGTM: Localization entries follow established patterns.The new Cisco Intersight URL localization keys follow the same validation message pattern as existing URL fields (e.g., Tang Server URL at line 816).
libs/ui-lib/lib/common/components/clusterConfiguration/DownloadIso.tsx (1)
25-25: LGTM: Clean integration of optional Cisco URL parameter.The optional
ciscoUrlprop is properly threaded through to the link construction function with appropriate fallback behavior when undefined.Also applies to: 41-41, 48-48
libs/ui-lib/lib/cim/hooks/index.tsx (1)
2-2: LGTM: Standard export addition.libs/ui-lib/lib/cim/hooks/useAgentServiceConfig.tsx (1)
5-15: LGTM: Clean hook implementation following established patterns.The hook properly configures Kubernetes resource watching for AgentServiceConfig with correct GVK and type safety.
libs/ui-lib/lib/common/components/ui/formik/InputField.tsx (2)
35-35: LGTM: Input type prop enables field type flexibility.Adding the
typeprop with a default value of'text'maintains backward compatibility while enabling different input types (url, password, email, etc.) needed for fields like the new Cisco Intersight URL input.Also applies to: 84-84
92-105: LGTM: Improved error and helper text rendering.Separating error and helper text into distinct
HelperTextItemcomponents improves clarity and follows PatternFly's recommended pattern for form field feedback.libs/ui-lib/lib/cim/components/modals/CimConfiguration/types.ts (2)
8-9: URL validation for ciscoIntersightURL field is correctly implemented.The
ciscoIntersightURLfield includes proper runtime validation inCimConfigurationModal.tsx(lines 103–120) that ensures the URL is required whenaddCiscoIntersightUrlis enabled and validates that it starts with "http://" or "https://" using theparseUrlutility.
3-10: No API updates required—all type changes are correctly implemented.All consumers of CimConfiguration types have already been updated to use the new type signatures. The codebase contains zero references to the old type names (
CimConfiguratioProps,CimConfigurationFormProps) or the removed resource operation props (createResource,getResource,listResources,patchResource). Components correctly use the new types:CimConfigurationValues,CimConfigurationFormFieldsProps, andCimConfigurationModalProps.Likely an incorrect or invalid review comment.
libs/ui-lib/lib/cim/components/modals/AddHostModal.tsx (2)
30-35: Verify the annotation key matches the persist.ts usage.The annotation key
'ciscoIntersightURL'is used here to read the Cisco URL. Ensure this matches the key used when creating the AgentServiceConfig inpersist.ts(line 291), which also usesciscoIntersightURL. The keys appear consistent.The conditional
loaded && !errorcorrectly guards against accessing properties before the resource is loaded or when there's an error.
106-136: LGTM!The
GeneratingIsoDownloadcomponent correctly accepts the optionalciscoUrlprop and forwards it toDownloadIso. The type annotation is appropriate.libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationFormFields.tsx (2)
224-255: LGTM!The Cisco Intersight URL configuration section is well-implemented:
- Checkbox controls conditional visibility of the URL input
- Both fields are correctly disabled in edit mode (
isEdit)- The placeholder provides a helpful example URL
isRequiredaligns with the validation schema in the modal
60-191: LGTM!The form structure is well-organized with:
- Consistent PatternFly component usage
- Proper accessibility attributes (
aria-labelon popovers)- Secure external links (
rel="noreferrer"withtarget="_blank")- Storage fields correctly disabled in edit mode to prevent post-configuration changes
libs/ui-lib/lib/cim/types/models.tsx (2)
14-23: LGTM!The
AgentServiceConfigModelis correctly defined as cluster-scoped (namespaced: false), which aligns with the Kubernetes CRD for AgentServiceConfig in the agent-install.openshift.io API group.
47-56: ProvisioningModel is correctly configured as cluster-scoped.The model is correctly defined with
namespaced: falsefor themetal3.io/v1alpha1Provisioning resource, which is a singleton cluster-wide resource. Codebase usage confirms this configuration—operations fetch and patch the resource by name without a namespace.libs/ui-lib/lib/cim/components/modals/CimConfiguration/CimConfigurationModal.tsx (4)
98-121: Static analysis false positive - Yup's conditional validation API is correct.The Biome linter flags line 105 with "Do not add then to an object" (
noThenProperty). This is a false positive because Yup's.when()API usesthenas a method name for conditional schema definitions, not as a Promise-like thenable property. This is the standard Yup pattern for conditional validation.The validation schema correctly:
- Validates storage sizes with minimum values
- Conditionally requires
ciscoIntersightURLwhenaddCiscoIntersightUrlis true- Validates URL format using
parseUrlto ensurehttp://orhttps://protocol
43-71: LGTM!The
onConfigurehandler correctly:
- Resets error state and sets submitting before the async operation
- Conditionally passes
ciscoIntersightURLonly when the checkbox is enabled- Closes modal on success, keeps it open with error on failure
- Properly resets
isSubmittingvia Formik helpers on failure
80-96: LGTM!The
initialValuescorrectly:
- Provides sensible defaults (10Gi, 100Gi, 50Gi) when creating new config
- Reads existing values from
agentServiceConfigwhen editing- Derives
addCiscoIntersightUrlfrom annotation presence using!!coercion- Uses consistent annotation key
'ciscoIntersightURL'
138-151: Consider if button should be disabled when no changes are made in edit mode.The Configure button disable logic checks
isEdit && configureLoadBalancerInitial === values.configureLoadBalancer, which only tracks load balancer changes. Since Cisco Intersight fields are disabled in edit mode (isDisabled={isEdit}in form fields), this is currently correct—the only editable field in edit mode is the load balancer checkbox.However, if future edits allow more fields to be modified, this logic would need updating.
libs/ui-lib/lib/cim/components/modals/CimConfiguration/persist.ts (5)
1-19: LGTM!The import restructuring to use
@openshift-console/dynamic-plugin-sdkoperations (k8sCreate,k8sGet,k8sListItems,k8sPatch) with locally-defined models provides a cleaner, more consistent approach to Kubernetes resource operations.
253-298: LGTM!The
createAgentServiceConfigfunction correctly handles the optionalciscoIntersightURL:
- Initializes with an empty annotations object (safe for K8s)
- Conditionally merges the annotation only when
ciscoIntersightURLis provided- Uses the consistent annotation key
ciscoIntersightURL- Properly uses
AgentServiceConfigModelfor the SDK create operation
134-146: LGTM!The
isIngressControllerfunction is cleanly refactored to use model-based SDK operations. The catch-all returningfalseis appropriate for an existence check—if the resource doesn't exist or there's any error accessing it, we treat it as "not present."
198-234: LGTM!The
patchProvisioningConfigurationfunction correctly:
- Uses
k8sGetwithout namespace (appropriate for cluster-scoped Provisioning resource)- Dynamically determines
op: 'add' | 'replace'based on existing field presence- Treats errors as warnings (
AlertVariant.warning) rather than blocking errors, allowing CIM setup to proceed
311-355: LGTM!The
onEnableCIMfunction is cleanly refactored:
- Accepts optional
ciscoIntersightURLand passes it through tocreateAgentServiceConfig- Simplified signature by removing resource-helper function parameters
- Internal calls now use the model-based SDK operations directly
- Maintains existing conditional logic for platform-specific provisioning configuration
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jgyselov, openshift-cherrypick-robot The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
openshift-merge-bot
bot
merged commit 60b2e0f
into
openshift-assisted:master
3 participants
This is an automated cherry-pick of #3282
/assign jgyselov
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.