OCPBUGS-75072 | OVE UI: Above-the-sea UI QE test#3382
OCPBUGS-75072 | OVE UI: Above-the-sea UI QE test#3382asmasarw wants to merge 1 commit intoopenshift-assisted:masterfrom
Conversation
openshift-ci
bot
added
the
size/L
📝 WalkthroughWalkthroughThis PR introduces a comprehensive validation schemas module with 30+ Yup-based validators for cluster configuration (names, SSH keys, IPs, VIPs, DNS, networks, proxies, and more), extends the cluster wizard context to persist pull secrets in disconnected mode, and refactors the OptionalConfigurationsStep component to support form rehydration from infrastructure environment data. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Form as OptionalConfigurationsStep
participant Context as ClusterWizardContext
participant API as Backend/InfraEnv
User->>Form: Navigate to OptionalConfigurations
Form->>API: Fetch current InfraEnv data
API-->>Form: Return InfraEnv with pullSecretSet
Form->>Form: infraEnvToFormValues() - Rehydrate form<br/>with stored pull secret
Form->>Form: Formik initializes with<br/>prior pull secret value
User->>Form: Edit & Submit form
Form->>Form: Validate using new schemas
Form->>API: POST/PATCH updated config
API-->>Form: Success response
Form->>Context: setDisconnectedFormPullSecret(pullSecret)
Context-->>Form: Store in context state
User->>Form: Click Next
Form->>Context: Trigger form submission<br/>& navigate
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
asmasarw
force-pushed
the
fix/above-the-sea
branch
from
0f8d026 to
eb49327
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts`:
- Around line 439-457: The DNS validation rejects uppercase letters because
DNS_NAME_REGEX is case-sensitive; update baseDomainValidationSchema to handle
case-insensitivity by either making DNS_NAME_REGEX case-insensitive (add the /i
flag) or normalizing the input to lowercase before validating in both the custom
test and the .matches call (i.e., use value?.toLowerCase() inside the test and
ensure matches is run against a lowercased value), referencing
baseDomainValidationSchema and DNS_NAME_REGEX so the change is made consistently
in both places.
🧹 Nitpick comments (5)
libs/ui-lib/lib/common/components/ui/formik/_validationSchemas.test.ts (1)
323-349: Consider adding more test cases to cover the new DNS regex validation.The valid/invalid lists are thin for the newly added
.matches(DNS_NAME_REGEX, …)check. Values like'co','1c','123.456','-a.com', or'a.com.'would help confirm the regex rejects single-label names, numeric-only TLDs, leading hyphens, and trailing dots. This improves confidence that the schema behaves correctly at the boundary.Proposed additional test cases
- const valid = ['a.com', 'b.example.com']; + const valid = ['a.com', 'b.example.com', 'sub.domain.example.org']; const invalid = [ ' ', 'a g', 'iamnotavaliddnsdomain-iamnotavaliddnsdomain-iamnotavaliddnsdomain', + 'co', // single-label, no dot + '1c', // single-label, no dot + '123.456', // numeric TLD + '-a.com', // leading hyphen in label + 'a.com.', // trailing dot ];libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (3)
71-76: Remove duplicate JSDoc block.There are two consecutive JSDoc comments (lines 71–73 and 74–76) for the same function. The first is a subset of the second.
Proposed fix
-/** - * Rehydrate form values from infraEnv (e.g. when navigating back from Review step) - */ -/** - * Rehydrate form values from infraEnv and optional stored pull secret (API does not return pull secret). - */ +/** + * Rehydrate form values from infraEnv and optional stored pull secret (API does not return pull secret). + */
193-193: Unnecessary alias and type assertion.
setDisconnectedFormPullSecretalready has the type(value: string | undefined) => voidfrom the context. Theascast and rename tosetPullSecretStoredadd indirection without benefit.Use context setter directly
- const setPullSecretStored = setDisconnectedFormPullSecret as (value: string | undefined) => void;Then replace
setPullSecretStored(...)calls on lines 261 and 278 withsetDisconnectedFormPullSecret(...).
195-202: Initial state and useEffect are partially redundant.Line 196 initializes
editPullSecretwith!!disconnectedInfraEnv?.pullSecretSet, and theuseEffecton lines 198–202 sets it totruewhen the same condition holds. The effect is only additive (never sets tofalse), so it handles late-arrivingdisconnectedInfraEnvupdates. This is fine, but note the effect will never reseteditPullSecretback tofalseif the infraEnv changes to one withoutpullSecretSet. If that's intentional, consider adding a brief comment.libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts (1)
439-453: The space check uses a type assertion that could be avoided.Line 445:
(value as string)— thevalueparameter isstring | undefined. The regex.test()will coerceundefinedto"undefined"string, which would incorrectly pass the space check but then fail on label length. Consider using(value ?? '')instead for safety.Proposed fix
- if (/\s/.test(value as string)) { + if (/\s/.test(value ?? '')) {
|
/retest |
asmasarw
force-pushed
the
fix/above-the-sea
branch
from
154fa2c to
d7c26ab
Compare
| const [disconnectedFormPullSecret, setDisconnectedFormPullSecret] = React.useState< | ||
| string | undefined | ||
| >(undefined); |
There was a problem hiding this comment.
No need for explicit undefined as TS will correctly infer the type
| const [disconnectedFormPullSecret, setDisconnectedFormPullSecret] = React.useState< | |
| string | undefined | |
| >(undefined); | |
| const [disconnectedFormPullSecret, setDisconnectedFormPullSecret] = React.useState< | |
| string | |
| >(); |
| const storedPullSecret: string | undefined = | ||
| typeof disconnectedFormPullSecret === 'string' ? disconnectedFormPullSecret : undefined; | ||
| const initialValues: OptionalConfigurationsFormValues = disconnectedInfraEnv | ||
| ? infraEnvToFormValues(disconnectedInfraEnv, storedPullSecret) | ||
| : DEFAULT_INITIAL_VALUES; |
There was a problem hiding this comment.
The whole storedPullSecret logic does essentially..nothing ? We can pass disconnectedFormPullSecret directly
| const storedPullSecret: string | undefined = | |
| typeof disconnectedFormPullSecret === 'string' ? disconnectedFormPullSecret : undefined; | |
| const initialValues: OptionalConfigurationsFormValues = disconnectedInfraEnv | |
| ? infraEnvToFormValues(disconnectedInfraEnv, storedPullSecret) | |
| : DEFAULT_INITIAL_VALUES; | |
| const initialValues: OptionalConfigurationsFormValues = disconnectedInfraEnv | |
| ? infraEnvToFormValues(disconnectedInfraEnv, disconnectedFormPullSecret) | |
| : DEFAULT_INITIAL_VALUES; |
| @@ -321,7 +321,7 @@ describe('validationSchemas', () => { | |||
| }); | |||
|
|
|||
| test('baseDomainNameValidationSchema', async () => { | |||
| const valid = ['a.com', 'co', '1c']; | |||
| const valid = ['a.com', 'b.example.com']; | |||
There was a problem hiding this comment.
Backend allows the co and 1c. They use different regex for validation
Must match regex [^([a-z\d]([\-]*[a-z\d]+)*\.)+[a-z\d]+([\-]*[a-z\d]+)+$], be no more than 255 characters, and not be in dotted decimal format (##.##.##.##)
| // Restore "Edit pull secret" checkbox when rehydrating from Review step (API only exposes pullSecretSet, not the secret value) | ||
| const [editPullSecret, setEditPullSecret] = React.useState(!!disconnectedInfraEnv?.pullSecretSet); | ||
|
|
||
| React.useEffect(() => { | ||
| if (disconnectedInfraEnv?.pullSecretSet) { | ||
| setEditPullSecret(true); | ||
| } | ||
| }, [disconnectedInfraEnv?.pullSecretSet]); |
There was a problem hiding this comment.
if the pullsecret is set, it does not mean edit pull secret should be checked (they could have used the default pullsecret).
We should store the state of the checkbox in the context as well
| return ( | ||
| <Formik | ||
| initialValues={initialValues} | ||
| enableReinitialize |
There was a problem hiding this comment.
this can be problematic, we do not want to reset the form when user is editing it. Lets remove it
| enableReinitialize |
|
/retest-required |
asmasarw
force-pushed
the
fix/above-the-sea
branch
from
d7c26ab to
1a52f1b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: asmasarw 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 |
openshift-ci
bot
added
size/XL
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libs/ui-lib/lib/common/validationSchemas/_validationSchemas.test.ts (1)
364-427:⚠️ Potential issue | 🟠 Major
dualStackValidationSchematests passtasopenshiftVersion— version strings are silently ignored.As noted in the schema file review,
dualStackValidationSchema('machine networks', t, '4.11')assigns the mocktfunction toopenshiftVersionand discards'4.11'. The version-specific behavior is not actually being tested — the tests pass by coincidence. Fix the call sites once the schema signature is corrected.
🤖 Fix all issues with AI agents
In `@libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts`:
- Around line 614-624: The isIPorDN function unsafely casts value to string for
.match which will throw if value is undefined; update isIPorDN to first guard
that value is a non-empty string (e.g., typeof value === 'string' &&
value.length > 0) before calling value.match(DNS_NAME_REGEX), and if the guard
fails return false (or skip to ipValidationSchema only if value is present),
keeping references to DNS_NAME_REGEX and ipValidationSchema.validateSync; remove
the (value as string) assertion and ensure undefined is handled safely to avoid
TypeError.
- Around line 103-113: The file defines duplicate Yup schemas (e.g.,
sshPublicKeyValidationSchema and dualStackValidationSchema) as plain schema
objects that shadow the canonical function-based validation exports (which
expect signatures like (t: TFunction) or (field: string, t: TFunction,
openshiftVersion?: string)); remove these duplicates from
libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts and restore
imports to use the canonical function-based validators from
libs/ui-lib/lib/common/validationSchemas/ (or re-export the canonical builders)
so symbols like sshPublicKeyValidationSchema and dualStackValidationSchema keep
their original function signatures and localization parameter.
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`:
- Line 189: The local alias uses an unnecessary type assertion; replace the
casted assignment so that setPullSecretStored is assigned directly from
setDisconnectedFormPullSecret (i.e., change the line creating
setPullSecretStored to use const setPullSecretStored =
setDisconnectedFormPullSecret;), removing the "as (value: string | undefined) =>
void" assertion since ClusterWizardContext already types
setDisconnectedFormPullSecret appropriately.
🧹 Nitpick comments (3)
libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx (2)
72-77: Duplicate JSDoc comment.Lines 72–74 are a redundant subset of the comment on lines 75–77. Remove the first block.
Proposed fix
-/** - * Rehydrate form values from infraEnv (e.g. when navigating back from Review step) - */ /** * Rehydrate form values from infraEnv and optional stored pull secret (API does not return pull secret). */
192-198: Initial state and useEffect are partially redundant.
editPullSecretis initialized to!!disconnectedInfraEnv?.pullSecretSeton line 192. TheuseEffecton lines 194–198 only ever sets it totrue(neverfalse), so it's only useful ifdisconnectedInfraEnvarrives asynchronously after mount. If that's the expected case, this is fine — just noting the one-directional nature of the effect (it can never reset the checkbox to unchecked).libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts (1)
260-270:alwaysRequiredandgetArrayIndexFromPathare not exported but used internally — consider exportinggetArrayIndexFromPathif needed elsewhere.
getArrayIndexFromPathis used insidevipBroadcastValidationSchemaandvipNoSuffixValidationSchema. If other modules need to parse array indices from Yup paths, it should be exported. Currently it's not exported, which is fine if usage is limited to this file.
| export const sshPublicKeyValidationSchema = Yup.string().test( | ||
| 'ssh-public-key', | ||
| 'SSH public key must consist of "[TYPE] key [[EMAIL]]", supported types are: ssh-rsa, ssh-ed25519, ecdsa-[VARIANT]. A single key can be provided only.', | ||
| (value?: string) => { | ||
| if (!value) { | ||
| return true; | ||
| } | ||
|
|
||
| return !!trimSshPublicKey(value).match(SSH_PUBLIC_KEY_REGEX); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how these schemas are re-exported from the common barrel
echo "=== Checking barrel exports in common/index.ts ==="
find libs/ui-lib/lib/common -maxdepth 1 -name 'index.ts' -exec cat {} \; | head -100
echo ""
echo "=== Look for all validationSchemas-related files ==="
fd -t f '(validationSchema|validation)' libs/ui-lib/lib/common --type ts
echo ""
echo "=== Check if sshPublicKeyValidationSchema is exported as a function ==="
rg -n 'sshPublicKeyValidationSchema' libs/ui-lib/lib/common -A 2 -B 2 --type ts | head -40
echo ""
echo "=== Check function signature patterns - look for function wrappers ==="
rg -n 'export const.*ValidationSchema.*=.*=>|export const.*ValidationSchema.*=.*\(' libs/ui-lib/lib/common --type ts | head -30Repository: openshift-assisted/assisted-installer-ui
Length of output: 7195
🏁 Script executed:
#!/bin/bash
echo "=== Content of libs/ui-lib/lib/common/validationSchemas directory ==="
ls -la libs/ui-lib/lib/common/validationSchemas/ | head -20
echo ""
echo "=== Check if there's an index.ts in validationSchemas ==="
cat libs/ui-lib/lib/common/validationSchemas/index.ts 2>/dev/null | head -50
echo ""
echo "=== Count of schemas in each location ==="
echo "Schemas in validationSchemas/ (canonical):"
rg 'export const.*ValidationSchema.*=' libs/ui-lib/lib/common/validationSchemas --count
echo ""
echo "Schemas in formik/validationSchemas.ts (duplicate/new):"
rg 'export const.*ValidationSchema.*=' libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts --count
echo ""
echo "=== Verify dualStackValidationSchema signature mismatch ==="
echo "In networkingValidation.tsx:"
rg 'export const dualStackValidationSchema' libs/ui-lib/lib/common/validationSchemas/networkingValidation.tsx -A 1
echo ""
echo "In formik/validationSchemas.ts:"
rg 'export const dualStackValidationSchema' libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts -A 1Repository: openshift-assisted/assisted-installer-ui
Length of output: 2254
Delete the duplicate validation schemas from libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts — they shadow canonical definitions with incompatible signatures.
This file duplicates 28+ validation schemas that already exist in the canonical location (libs/ui-lib/lib/common/validationSchemas/), but defines them as plain Yup objects instead of functions accepting t: TFunction for localization.
The canonical definitions (in individual files like sshValidation.tsx, proxyValidation.tsx, etc.) are the authoritative versions exported via the barrel and used throughout the codebase. The new duplicates break expected function signatures:
- All schemas: Defined as plain
Yup.string(),Yup.array()objects; canonical versions are functions(t: TFunction) => Yup.string(), etc. dualStackValidationSchema: Missing thet: TFunctionparameter entirely — canonical signature is(field: string, t: TFunction, openshiftVersion?: string), but new definition is(field: string, openshiftVersion?: string).
These duplicate definitions will cause TypeError: ... is not a function if imported and called with (t), and they are not wired into the barrel export, making them dead code.
🤖 Prompt for AI Agents
In `@libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts` around
lines 103 - 113, The file defines duplicate Yup schemas (e.g.,
sshPublicKeyValidationSchema and dualStackValidationSchema) as plain schema
objects that shadow the canonical function-based validation exports (which
expect signatures like (t: TFunction) or (field: string, t: TFunction,
openshiftVersion?: string)); remove these duplicates from
libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts and restore
imports to use the canonical function-based validators from
libs/ui-lib/lib/common/validationSchemas/ (or re-export the canonical builders)
so symbols like sshPublicKeyValidationSchema and dualStackValidationSchema keep
their original function signatures and localization parameter.
| const isIPorDN = (value?: string, dnsRegex = DNS_NAME_REGEX) => { | ||
| if ((value as string).match(dnsRegex)) { | ||
| return true; | ||
| } | ||
| try { | ||
| ipValidationSchema.validateSync(value); | ||
| return true; | ||
| } catch (err) { | ||
| return false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Unsafe type assertion — value as string when value may be undefined.
Line 615 casts value to string without a null check. While current callers always pass split results (which are strings), a direct call like isIPorDN(undefined) would throw TypeError: Cannot read properties of undefined (reading 'match'). Add a guard.
Proposed fix
const isIPorDN = (value?: string, dnsRegex = DNS_NAME_REGEX) => {
+ if (!value) {
+ return false;
+ }
- if ((value as string).match(dnsRegex)) {
+ if (value.match(dnsRegex)) {
return true;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isIPorDN = (value?: string, dnsRegex = DNS_NAME_REGEX) => { | |
| if ((value as string).match(dnsRegex)) { | |
| return true; | |
| } | |
| try { | |
| ipValidationSchema.validateSync(value); | |
| return true; | |
| } catch (err) { | |
| return false; | |
| } | |
| }; | |
| const isIPorDN = (value?: string, dnsRegex = DNS_NAME_REGEX) => { | |
| if (!value) { | |
| return false; | |
| } | |
| if (value.match(dnsRegex)) { | |
| return true; | |
| } | |
| try { | |
| ipValidationSchema.validateSync(value); | |
| return true; | |
| } catch (err) { | |
| return false; | |
| } | |
| }; |
🤖 Prompt for AI Agents
In `@libs/ui-lib/lib/common/components/ui/formik/validationSchemas.ts` around
lines 614 - 624, The isIPorDN function unsafely casts value to string for .match
which will throw if value is undefined; update isIPorDN to first guard that
value is a non-empty string (e.g., typeof value === 'string' && value.length >
0) before calling value.match(DNS_NAME_REGEX), and if the guard fails return
false (or skip to ipValidationSchema only if value is present), keeping
references to DNS_NAME_REGEX and ipValidationSchema.validateSync; remove the
(value as string) assertion and ensure undefined is handled safely to avoid
TypeError.
| disconnectedFormPullSecret, | ||
| setDisconnectedFormPullSecret, | ||
| } = useClusterWizardContext(); | ||
| const setPullSecretStored = setDisconnectedFormPullSecret as (value: string | undefined) => void; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for setDisconnectedFormPullSecret definition and usage
rg 'setDisconnectedFormPullSecret' --type=ts --type=tsx -B 2 -A 2Repository: openshift-assisted/assisted-installer-ui
Length of output: 111
🏁 Script executed:
# Search for setDisconnectedFormPullSecret definition and usage without file type restriction
rg 'setDisconnectedFormPullSecret' -B 2 -A 2Repository: openshift-assisted/assisted-installer-ui
Length of output: 2738
🏁 Script executed:
# Confirm the exact context interface to ensure the type is as expected
rg -A 5 'setDisconnectedFormPullSecret: ' libs/ui-lib/lib/ocm/components/clusterWizard/ClusterWizardContext.tsxRepository: openshift-assisted/assisted-installer-ui
Length of output: 301
Remove the unnecessary type assertion — the context already provides the correct type.
The setDisconnectedFormPullSecret is already typed as (value: string | undefined) => void in ClusterWizardContext.tsx, making the as cast on line 189 redundant. Simply use const setPullSecretStored = setDisconnectedFormPullSecret; instead.
🤖 Prompt for AI Agents
In
`@libs/ui-lib/lib/ocm/components/clusterWizard/disconnected/OptionalConfigurationsStep.tsx`
at line 189, The local alias uses an unnecessary type assertion; replace the
casted assignment so that setPullSecretStored is assigned directly from
setDisconnectedFormPullSecret (i.e., change the line creating
setPullSecretStored to use const setPullSecretStored =
setDisconnectedFormPullSecret;), removing the "as (value: string | undefined) =>
void" assertion since ClusterWizardContext already types
setDisconnectedFormPullSecret appropriately.
|
@asmasarw: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
3 participants
Include the following fixes:
Summary by CodeRabbit
New Features
Style
Tests