NETOBSERV-2486: Add Network Observability Operator support#3351
NETOBSERV-2486: Add Network Observability Operator support#3351jpinsonneau wants to merge 6 commits intoopenshift-assisted:masterfrom
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jpinsonneau The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Welcome @jpinsonneau! It looks like this is your first PR to openshift-assisted/assisted-installer-ui 🎉 |
|
Hi @jpinsonneau. Thanks for your PR. I'm waiting for a openshift-assisted member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
openshift-ci
bot
added
needs-ok-to-test
|
Warning Rate limit exceeded@jpinsonneau has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 0 minutes and 4 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds Network Observability operator support (constants, docs link, spec/description, API/service), UI to fetch and configure operator properties (new form + checkbox behavior), extends OperatorsValues to carry operatorProperties, updates tests, and adds unbound host status variants plus new network-observability validation IDs/types. Changes
Sequence DiagramsequenceDiagram
autonumber
participant User
participant OperatorCheckbox
participant OperatorsService
participant OperatorsAPI
participant Server
participant OperatorPropertiesForm
participant Formik
Note over OperatorCheckbox,OperatorsService: Operator selection + properties fetch (new flow)
User->>OperatorCheckbox: checks operator
OperatorCheckbox->>OperatorCheckbox: set isChecked=true, show loader
OperatorCheckbox->>OperatorsService: getOperatorProperties(name)
OperatorsService->>OperatorsAPI: getProperties(name)
OperatorsAPI->>Server: GET /v2/supported-operators/{name}
Server-->>OperatorsAPI: 200 OperatorProperties (metadata)
OperatorsAPI-->>OperatorsService: OperatorProperties
OperatorsService-->>OperatorCheckbox: OperatorProperties
OperatorCheckbox->>OperatorPropertiesForm: render with properties
OperatorPropertiesForm->>OperatorPropertiesForm: sanitize/init defaults
User->>OperatorPropertiesForm: edit property
OperatorPropertiesForm->>Formik: set operatorProperties[name] = JSON.stringify(...)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
jpinsonneau
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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. |
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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 (6)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
172-192: Properties are not refetched when operator is unchecked and re-checked.When an operator is unchecked and then checked again,
operatorPropertiesstill contains the previously fetched data, so the fetch won't trigger. This may be intentional (caching), but if properties can change server-side, consider clearingoperatorPropertieswhen the operator is unchecked.Additionally, consider adding a cleanup function to handle unmount during pending requests:
♻️ Optional: Add request cancellation
React.useEffect(() => { + let cancelled = false; if (isChecked && operatorProperties.length === 0 && !propertiesLoading) { setPropertiesLoading(true); OperatorsService.getOperatorProperties(operatorId) .then((properties) => { - setOperatorProperties(properties); + if (!cancelled) { + setOperatorProperties(properties); + } }) .catch((error) => { - handleApiError(error, () => - addAlert({ - title: 'Failed to fetch operator properties', - message: getApiErrorMessage(error), - }), - ); + if (!cancelled) { + handleApiError(error, () => + addAlert({ + title: 'Failed to fetch operator properties', + message: getApiErrorMessage(error), + }), + ); + } }) .finally(() => { - setPropertiesLoading(false); + if (!cancelled) { + setPropertiesLoading(false); + } }); } + return () => { + cancelled = true; + }; }, [isChecked, operatorId, operatorProperties.length, propertiesLoading, addAlert]);libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (5)
141-167: Potential infinite loop risk in initialization useEffect.The useEffect at line 141 calls
setFieldValue('operatorProperties', ...)which updatesvalues.operatorProperties. However,values.operatorPropertiesis also in the dependency array (line 167). This could potentially trigger re-renders, though the guard condition on line 142 should prevent an infinite loop.Consider extracting the initialization check to a ref to make the intent clearer:
♻️ Suggested improvement
+ const initializedRef = React.useRef(false); + React.useEffect(() => { - if (!values.operatorProperties || !values.operatorProperties[operatorName]) { + if (!initializedRef.current && (!values.operatorProperties || !values.operatorProperties[operatorName])) { + initializedRef.current = true; const defaults: Record<string, unknown> = {}; // ... rest of initialization } - }, [operatorName, properties, setFieldValue, values.operatorProperties]); + }, [operatorName, properties, setFieldValue, values.operatorProperties]);
275-297: Consider extracting the NumberInput value computation.The IIFE inside the
valueprop is complex and harder to read. Consider extracting it to a helper or computing it in the component body.♻️ Suggested refactor
+ const getNumberValue = ( + currentValue: unknown, + defaultValue: unknown + ): number => { + if (typeof currentValue === 'number') return currentValue; + if (typeof currentValue === 'string') { + const parsed = parseInt(currentValue, 10); + if (!isNaN(parsed)) return parsed; + } + if (typeof defaultValue === 'number') return defaultValue; + if (typeof defaultValue === 'string') { + const parsed = parseInt(defaultValue, 10); + if (!isNaN(parsed)) return parsed; + } + return 0; + }; // In the JSX: <NumberInput id={fieldId} - value={(() => { - // ... IIFE logic - })()} + value={getNumberValue(currentValue, defaultValue)} // ... />
232-237: Minor:onTogglecallback can be simplified.♻️ Minor simplification
<ExpandableSection toggleText={`Configure ${operatorName} properties`} - onToggle={() => setIsExpanded(!isExpanded)} + onToggle={(_event, expanded) => setIsExpanded(expanded)} isExpanded={isExpanded} data-testid={`operator-properties-${operatorId}`} >This uses the expanded state from the callback rather than toggling based on current state, which is slightly more reliable.
146-148: Use the correct type definition or properly transform the API response to match the type definition.The code imports
OperatorPropertyfrom the camelCase type definition but accesses both snake_case and camelCase properties viaas any. This mismatch suggests the API response format differs from the imported type. Either import from the auto-generated SDK types that use snake_case properties (which match the OpenAPI spec), or ensure the type definition includes both formats. The current workaround at lines 146-147 and 248-249 bypasses TypeScript safety unnecessarily.
326-338: Simplify TextInput onChange handler to remove unnecessary type checking.PatternFly 6.2.2's TextInput
onChangecallback signature is(event: React.FormEvent<HTMLInputElement>, value: string) => void. The second parameter is already the string value. The complex type checking for event objects is unnecessary and inconsistent with all other TextInput usages in the codebase (e.g.,ClusterEventsToolbar.tsx,MultiSelectField.tsx,RichInputField.tsx), which directly use the value parameter.Suggested simplification
onChange={(_, value) => { // Extract value from event object if needed // PatternFly TextInput onChange can pass either a string or an event object let stringValue = ''; if (value && typeof value === 'object' && 'currentTarget' in value) { const event = value as React.FormEvent<HTMLInputElement>; stringValue = event.currentTarget.value; } else if (typeof value === 'string') { stringValue = value; } else { stringValue = String(value ?? ''); } updateProperty(property.name || '', stringValue); }} + onChange={(_, value) => updateProperty(property.name || '', value)}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
libs/types/assisted-installer-service.d.tslibs/ui-lib/lib/common/api/assisted-service/OperatorsAPI.tslibs/ui-lib/lib/common/components/operators/operatorDescriptions.tsxlibs/ui-lib/lib/common/components/operators/operatorSpecs.tsxlibs/ui-lib/lib/common/config/constants.tslibs/ui-lib/lib/common/config/docs_links.tslibs/ui-lib/lib/common/types/clusters.tslibs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.test.tslibs/ui-lib/lib/ocm/services/OperatorsService.tsx
🧰 Additional context used
🧠 Learnings (3)
📚 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.tslibs/ui-lib/lib/common/components/operators/operatorDescriptions.tsxlibs/ui-lib/lib/common/components/operators/operatorSpecs.tsxlibs/ui-lib/lib/common/config/constants.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/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsxlibs/ui-lib/lib/common/components/operators/operatorSpecs.tsxlibs/ui-lib/lib/common/config/constants.ts
📚 Learning: 2025-12-17T09:08:18.279Z
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:18.279Z
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/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsxlibs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.test.tslibs/types/assisted-installer-service.d.ts
🧬 Code graph analysis (5)
libs/ui-lib/lib/common/api/assisted-service/OperatorsAPI.ts (1)
libs/types/assisted-installer-service.d.ts (1)
OperatorProperties(2391-2391)
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (1)
libs/ui-lib/lib/common/selectors/clusterSelectors.ts (1)
selectOlmOperators(40-44)
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
libs/types/assisted-installer-service.d.ts (1)
OperatorProperties(2391-2391)
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (3)
libs/ui-lib/lib/common/config/constants.ts (1)
OPERATOR_NAME_NETWORK_OBSERVABILITY(353-353)libs/ui-lib/lib/common/components/operators/operatorDescriptions.tsx (1)
DESCRIPTION_NETWORK_OBSERVABILITY(78-79)libs/ui-lib/lib/common/config/docs_links.ts (1)
getNetworkObservabilityLink(241-244)
libs/ui-lib/lib/common/config/constants.ts (1)
libs/types/assisted-installer-service.d.ts (1)
ClusterValidationId(643-693)
🔇 Additional comments (19)
libs/types/assisted-installer-service.d.ts (3)
1108-1109: LGTM - Feature support level addition.The
NETWORK_OBSERVABILITYfeature support level ID follows the established naming pattern and is correctly placed.
1174-1202: LGTM - Host status union expansion.The new unbound host statuses (
insufficient-unbound,disabled-unbound,discovering-unbound,reclaiming,reclaiming-rebooting) are correctly added and consistent with theHostRegistrationResponse.statusdefinition at lines 1381-1409.
691-693: This duplication is intentional and follows the established pattern in the codebase. The results show that most-requirements-satisfiedvalidations appear in bothClusterValidationIdandHostValidationId, including similar validations likenode-feature-discovery-requirements-satisfied,nvidia-gpu-requirements-satisfied,openshift-logging-requirements-satisfied, and others. The presence ofnetwork-observability-host-requirements-satisfiedin both types aligns with this systematic design where cluster-level and host-level validations coexist for feature requirements.Likely an incorrect or invalid review comment.
libs/ui-lib/lib/common/types/clusters.ts (1)
61-65: LGTM - OperatorsValues type extension.The
operatorPropertiesfield with typeRecord<string, string>is appropriate for storing JSON-serialized operator configuration by operator name. The inline comment provides helpful context.libs/ui-lib/lib/common/components/operators/operatorDescriptions.tsx (1)
77-79: LGTM - Network Observability description.The description accurately reflects the operator's functionality and follows the established pattern for other operator descriptions in this file.
libs/ui-lib/lib/ocm/services/OperatorsService.tsx (1)
10-13: LGTM - Service method addition.The
getOperatorPropertiesmethod follows the same pattern as the existinggetSupportedOperatorsmethod. Error handling is delegated to the caller, which is consistent with the service layer design.libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.test.ts (1)
49-54: LGTM - Test data structure updated.All test cases correctly include the new
operatorProperties: {}field to match the updatedOperatorsValuestype. The empty object is appropriate since these tests focus on operator selection counting logic, not properties handling.libs/ui-lib/lib/common/api/assisted-service/OperatorsAPI.ts (1)
13-17: LGTM - API method with proper URL encoding.Good implementation with
encodeURIComponent(operatorName)to safely handle operator names in the URL path. The method follows the established pattern and is properly typed.libs/ui-lib/lib/common/config/docs_links.ts (1)
240-244: LGTM - Documentation link function follows established pattern.The
getNetworkObservabilityLinkfunction correctly implements the documentation link pattern consistent with other operators that don't require version-specific fallback logic (MetalLB, OADP, Cluster Observability, NUMA Resources). The URL structure and version handling viagetDocsOpenshiftVersionare correct.libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx (1)
354-366: LGTM! Network Observability operator spec follows established patterns.The new operator entry is consistent with other operators in the Network category. It correctly uses the version-aware documentation link function and integrates with the feature support level system.
libs/ui-lib/lib/common/config/constants.ts (4)
353-353: LGTM! Operator constant follows naming conventions.The constant follows the established
OPERATOR_NAME_prefix pattern with kebab-case value.
153-154: LGTM! Host validation labels and hints added correctly.Both network-observability validation IDs are properly added to host validation mappings, consistent with the type definitions.
Also applies to: 228-229
266-267: LGTM! Cluster validation label added.The cluster validation label is correctly added. The type assertion is appropriate for the partial mapping pattern used here.
355-366: Consider whether Network Observability should be added tosingleClusterOperators.The
singleClusterOperatorsarray (lines 355-366) lists operators with specific single-cluster behavior. Other observability operators likecluster-observability,loki, andopenshift-loggingare included. Verify ifnetwork-observabilityshould also be added to this list based on its intended behavior.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
238-245: LGTM! Conditional rendering of properties form.The form is correctly shown only when the operator is checked and has properties. The disabled state is properly derived from viewer mode or disabled reason.
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx (3)
32-45: LGTM! Operator properties extraction is well-implemented.The function correctly extracts properties from OLM operators and handles undefined values safely.
Minor nit: Line 44 has a redundant
|| {}fallback sinceoperatorPropertiesis already initialized as{}on line 33.
98-104: LGTM! Submit handler correctly includes operator properties.The properties are properly included in the payload only when they exist for the operator.
124-130: Verify memoization behavior with cluster object.The
useMemodependency onclustermay cause unnecessary recalculations if the cluster object reference changes even when relevant data (monitoredOperators) hasn't changed. Consider if this impacts performance in practice.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (1)
25-101: Extensive sanitization suggests defensive coding against past issues.The
sanitizeValueandcleanPropertieshelpers contain extensive defensive checks against objects, functions, and event objects. While this is safe, the detailed comments about event object properties (isDefaultPrevented,nativeEvent, etc.) suggest this was added to fix a specific bug where event objects were accidentally stored.If the root cause has been addressed elsewhere (e.g., in the onChange handlers), this sanitization layer may be more defensive than necessary. However, keeping it as a safety net is reasonable given the JSON serialization requirements.
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
@libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx:
- Around line 103-115: The normalizeOperatorProperty function currently uses an
unsafe cast (prop as any) to access snake_case API fields; reconcile the
conflicting OperatorProperty definitions (the hand-written type in
libs/types/assisted-installer-service.d.ts vs the SDK model in
libs/sdks/.../operator-property.ts) so the code can rely on a single correct
shape, then remove the as any cast in normalizeOperatorProperty and update its
parameter type to accept the actual API shape (or create a distinct
ApiOperatorProperty type), and finally implement/route through a small
deserialization layer that maps api fields (data_type, default_value) to
internal camelCase fields (dataType, defaultValue) in normalizeOperatorProperty
without bypassing TypeScript checks.
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (2)
172-205: Consider using a more robust condition thanoperatorProperties.length === 0.The useEffect depends on
operatorProperties.length, which means if the array identity changes but the length remains 0, the effect won't re-run. This could cause issues if a fetch fails silently or returns an empty array.Consider tracking the fetch status with a dedicated flag (e.g.,
propertiesFetched) instead of relying on array length.♻️ Proposed refactor
- const [operatorProperties, setOperatorProperties] = React.useState<OperatorProperties>([]); - const [propertiesLoading, setPropertiesLoading] = React.useState(false); + const [operatorProperties, setOperatorProperties] = React.useState<OperatorProperties>([]); + const [propertiesLoading, setPropertiesLoading] = React.useState(false); + const [propertiesFetched, setPropertiesFetched] = React.useState(false); // Fetch operator properties when operator is selected React.useEffect(() => { - if (isChecked && operatorProperties.length === 0 && !propertiesLoading) { + if (isChecked && !propertiesFetched && !propertiesLoading) { setPropertiesLoading(true); let cancelled = false; OperatorsService.getOperatorProperties(operatorId) .then((properties) => { if (!cancelled) { setOperatorProperties(properties); + setPropertiesFetched(true); } }) .catch((error) => { if (!cancelled) { handleApiError(error, () => addAlert({ title: 'Failed to fetch operator properties', message: getApiErrorMessage(error), }), ); } }) .finally(() => { if (!cancelled) { setPropertiesLoading(false); } }); return () => { cancelled = true; }; } else if (!isChecked) { // Clear properties when operator is unchecked to allow refetch on re-check setOperatorProperties([]); + setPropertiesFetched(false); } - }, [isChecked, operatorId, operatorProperties.length, propertiesLoading, addAlert]); + }, [isChecked, operatorId, propertiesFetched, propertiesLoading, addAlert]);
251-258: Minor:operatorNameprop receives the same value asoperatorId.Line 254 passes
operatorName={operatorId}, which duplicates the value fromoperatorId. If the component doesn't need a separate name, consider usingoperatorIdinternally or deriving the display name from operator specs.libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (1)
25-62: Observation: Extensive defensive programming suggests potential architectural concern.The
sanitizeValuefunction includes detailed guards against event objects, DOM elements, and functions. While this implementation is correct, the need for such extensive sanitization may indicate that callers are passing unsanitized values (e.g., raw events instead ofevent.target.value).Consider whether the root cause can be addressed by ensuring values are properly extracted at the call site (e.g., in
updatePropertycallers).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
📚 Learning: 2025-12-17T09:08:18.279Z
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:18.279Z
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/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (3)
libs/types/assisted-installer-service.d.ts (1)
OperatorProperty(2392-2417)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-65)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFieldId(15-18)
🔇 Additional comments (3)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx (1)
19-19: LGTM!The new imports and state setup correctly support the operator properties fetching workflow.
Also applies to: 26-26, 34-36, 136-136, 143-144
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (2)
277-361: LGTM!The form rendering logic correctly handles different property types (boolean, integer, string) with appropriate PatternFly components and includes proper disabled state handling and test IDs.
336-341: Review comment is incorrect. PatternFly React v6'sNumberInput.onChangereceives only(event: React.FormEvent<HTMLInputElement>) => void, not a second value parameter. The current implementation at lines 336–341 correctly accessesevent.target.valueand is consistent with the project's ownNumberInputFieldwrapper component, which uses the same pattern.No changes needed.
Likely an incorrect or invalid review comment.
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
Outdated
Show resolved
Hide resolved
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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. |
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (3)
92-129: Consider reducing duplication betweencleanPropertiesandupdateProperty.Both functions iterate through properties and filter out non-primitives with similar logic (lines 97-120 vs. 259-281). Since
currentPropertiesis already cleaned in the memo (line 204), the defensive re-checks inupdatePropertymay be redundant—or intentionally guarding against race conditions.If the defensive re-sanitization is necessary, consider extracting the shared filtering logic into a reusable helper to reduce maintenance overhead.
♻️ Example refactor to share filtering logic
+// Shared helper to filter and sanitize a properties object +const filterToSerializableProperties = ( + props: Record<string, unknown> +): Record<string, string | number | boolean> => { + const filtered: Record<string, string | number | boolean> = {}; + for (const key in props) { + if (Object.prototype.hasOwnProperty.call(props, key) && typeof key === 'string') { + const value = props[key]; + if (typeof value === 'function') continue; + if (typeof value === 'object' && value !== null) continue; + filtered[key] = sanitizeValue(value); + } + } + return filtered; +}; + const cleanProperties = (props: Record<string, unknown>): Record<string, string | number | boolean> => { - const cleaned: Record<string, string | number | boolean> = {}; try { - for (const [key, value] of Object.entries(props)) { - if (typeof key !== 'string') continue; - if (typeof value === 'function') continue; - if (typeof value === 'object' && value !== null) continue; - const sanitized = sanitizeValue(value); - cleaned[key] = sanitized; - } + const cleaned = filterToSerializableProperties(props); JSON.stringify(cleaned); + return cleaned; } catch (error) { console.warn('Failed to clean properties:', error); return {}; } - return cleaned; }; // In updateProperty: - const newProperties: Record<string, string | number | boolean> = {}; - for (const key in currentProperties) { - if (Object.prototype.hasOwnProperty.call(currentProperties, key) && typeof key === 'string') { - const existingValue = currentProperties[key]; - if (typeof existingValue === 'function') continue; - if (typeof existingValue === 'object' && existingValue !== null) continue; - newProperties[key] = sanitizeValue(existingValue); - } - } + const newProperties = filterToSerializableProperties(currentProperties); newProperties[propertyName] = sanitizedValue;Also applies to: 249-306
218-247: Initialization logic works correctly for typical use, minor edge case to note.The
initializationRefprevents re-initialization after the first render, which is appropriate for the main use case (operator checked → component mounts → defaults initialized → operator unchecked → component unmounts → operator checked again → component remounts with fresh ref).However, there's an edge case: if
values.operatorProperties[operatorId]is cleared by an external action while the component remains mounted, re-initialization won't occur becauseinitializationRef.currentstaystrue(line 220). This seems unlikely in practice but worth noting for future maintenance.
353-379: Consider bounds forNumberInputbased on property metadata.The
onMinushandler prevents negative values withMath.max(0, numValue - 1)(line 362), butonPlushas no upper limit (line 369). While this asymmetry might be intentional (e.g., sampling rates can be any positive integer), consider whether the API could provide optionalmin/maxvalues in the property metadata to enforce consistent bounds on both ends.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsxlibs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 Learning: 2025-12-17T09:08:18.279Z
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:18.279Z
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/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
📚 Learning: 2025-09-02T08:03:57.204Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.204Z
Learning: In the openshift-assisted/assisted-installer-ui repository, non-English translations are handled separately from the main development process. When adding new translation keys to English locale files, it's expected that non-English locale files (es, fr, ja, ko, zh) may not immediately contain the corresponding translations, as these are managed through a different workflow.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
📚 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/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
🧬 Code graph analysis (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (4)
libs/types/assisted-installer-service.d.ts (1)
OperatorProperty(2392-2417)libs/ui-lib/lib/ocm/services/InfraEnvIdsCacheService.ts (1)
key(32-35)libs/ui-lib/lib/common/types/clusters.ts (1)
OperatorsValues(61-65)libs/ui-lib/lib/common/components/ui/formik/utils.ts (1)
getFieldId(15-18)
🔇 Additional comments (3)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (3)
18-151: Type safety improvements look excellent.The distinct type definitions (
ApiOperatorPropertyfor snake_case API responses,NormalizedOperatorPropertyfor internal camelCase) and thenormalizeOperatorPropertydeserialization layer effectively address the previous review's type safety concerns. The targeted type assertions (as ApiOperatorProperty,as OperatorProperty) are a significant improvement over the previousas anycast, and the fallback logic handles both API contract and axios-case-converter scenarios cleanly.
47-129: Defensive sanitization approach is appropriate and well-documented.The extensive primitive-only checks in
sanitizeValueandcleanPropertiesare justified by the JSON serialization requirements explained in the comments. The explicit rejection of functions, event objects, and DOM elements (with detailed comments referencing specific properties likeisDefaultPrevented,nativeEvent, etc.) suggests this guards against real-world edge cases.
344-352: Input handlers correctly match PatternFly 6 API signatures.The
SwitchandTextInputonChange handlers properly destructure the PatternFly callback parameters (event and value/checked) and pass only primitives toupdateProperty. This aligns with the defensive sanitization strategy and prevents event objects from polluting the state.Also applies to: 380-388
|
@jpinsonneau: This pull request references NETOBSERV-2486 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 spike 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. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (1)
344-376: Consider consolidating NumberInput value computation logic.The
valueprop usescomputeNumberInputValue(line 347), but theonMinusandonPlushandlers duplicate similar parsing logic inline (lines 349-352, 359-362). For consistency and DRY, consider extracting the inline logic to reusecomputeNumberInputValueor creating a shared helper.The hardcoded minimum of 0 and lack of maximum (lines 353-365) are appropriately documented with comments suggesting future enhancement when API adds min/max metadata.
♻️ Potential refactor to consolidate logic
<NumberInput id={fieldId} value={computeNumberInputValue(currentValue, defaultValue)} onMinus={() => { - const numValue = - typeof currentValue === 'number' - ? currentValue - : parseInt(String(currentValue ?? defaultValue ?? 0), 10); + const numValue = computeNumberInputValue(currentValue, defaultValue); updateProperty(property.name || '', Math.max(0, numValue - 1)); }} onPlus={() => { - const numValue = - typeof currentValue === 'number' - ? currentValue - : parseInt(String(currentValue ?? defaultValue ?? 0), 10); + const numValue = computeNumberInputValue(currentValue, defaultValue); updateProperty(property.name || '', numValue + 1); }}
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
📚 Learning: 2025-12-17T09:08:18.279Z
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:18.279Z
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/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
📚 Learning: 2025-12-17T09:16:41.439Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3319
File: libs/ui-lib/lib/common/components/hosts/utils.ts:208-210
Timestamp: 2025-12-17T09:16:41.439Z
Learning: In libs/ui-lib/lib/common/components/hosts/utils.ts, the nodeLabels field on Host objects is guaranteed to be valid JSON because it's always constructed using JSON.stringify in libs/ui-lib/lib/cim/components/helpers/toAssisted.ts, making direct JSON.parse safe without additional error handling.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
📚 Learning: 2025-09-02T08:03:57.204Z
Learnt from: jgyselov
Repo: openshift-assisted/assisted-installer-ui PR: 3151
File: libs/ui-lib/lib/cim/components/ClusterDeployment/constants.ts:25-25
Timestamp: 2025-09-02T08:03:57.204Z
Learning: In the openshift-assisted/assisted-installer-ui repository, non-English translations are handled separately from the main development process. When adding new translation keys to English locale files, it's expected that non-English locale files (es, fr, ja, ko, zh) may not immediately contain the corresponding translations, as these are managed through a different workflow.
Applied to files:
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
📚 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/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx
🔇 Additional comments (7)
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorPropertiesForm.tsx (7)
18-45: LGTM: Well-designed type architecture.The separation between
ApiOperatorProperty(API contract with snake_case) andNormalizedOperatorProperty(internal camelCase) provides clear type boundaries. The component props accepting a union of both types offers flexibility for different transformation scenarios (raw API vs. axios-case-converter), and the documentation clearly explains the intent.
47-137: LGTM: Robust sanitization layer.The defensive sanitization functions (
sanitizeValue,filterToSerializableProperties,cleanProperties) provide strong guarantees for JSON serialization. The extensive type checks and error handling are well-justified by the comments and prevent runtime failures from corrupted state or unexpected input types.
139-187: LGTM: Clean deserialization layer.The
normalizeOperatorPropertyfunction properly handles both API contract (snake_case) and axios-transformed (camelCase) formats with clear preference for the API format. ThecomputeNumberInputValuehelper provides consistent number parsing with sensible fallbacks.
226-261: Initialization logic is sound with documented tradeoff.The
initializationRefprevents re-initialization on re-renders, which is the correct behavior for the typical mount/unmount cycle. The edge case comment (lines 227-232) transparently documents the limitation where external clearing ofoperatorProperties[operatorId]while the component remains mounted won't trigger re-initialization. This tradeoff is acceptable given the unlikely scenario and the complexity that full dependency tracking would introduce.
263-297: LGTM: Robust update handler with excellent error recovery.The
updatePropertyfunction implements defense-in-depth: sanitizes input, re-filters the base state, verifies serializability, and provides a minimal fallback if stringify fails. The outer try-catch prevents state corruption. This defensive approach is appropriate for maintaining JSON-serializable form state.
303-343: LGTM: Clean, accessible rendering logic.The expandable section with proper FormGroup structure, helper text, and data-testid attributes provides good accessibility and testability. The Switch component for boolean properties correctly passes the checked state to
updateProperty.
377-393: LGTM: TextInput and overall structure are well-implemented.The TextInput for string properties and the overall StackItem mapping provide a clean, extensible structure. The
data-testidattributes enable reliable testing, and the early return (lines 299-301) for empty properties is appropriate.
2 participants
Add Network Observability Operator UI support
Summary
This PR adds comprehensive UI support for the Network Observability Operator in the Assisted Installer UI. This includes operator selection, configuration form with property management, integration into the operator selection workflow, and proper TypeScript type definitions. The implementation enables users to configure the Network Observability Operator with custom properties like FlowCollector creation and eBPF sampling rates.
Key Features
Changes
New Components
OperatorPropertiesForm.tsx:Operator Integration
Operator Specs (
libs/ui-lib/lib/common/components/operators/operatorSpecs.tsx):Operator Selection (
libs/ui-lib/lib/ocm/components/clusterWizard/Operators.tsx):getOperatorsInitialValuesto extract and preserve operator properties from clusterhandleSubmitto include operator properties when updating clusteroperatorPropertiesfield toOperatorsValuestypeOperator Checkbox (
libs/ui-lib/lib/ocm/components/clusterConfiguration/operators/OperatorCheckbox.tsx):OperatorPropertiesFormcomponentOperators Service (
libs/ui-lib/lib/ocm/services/OperatorsService.tsx):API Integration
libs/ui-lib/lib/common/api/assisted-service/OperatorsAPI.ts:Tests
libs/ui-lib/lib/ocm/components/clusterWizard/OperatorsSelect.test.ts:User Experience
Operator Selection Flow
Property Form Features
Testing
assisted-ui.mp4
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.