-
Notifications
You must be signed in to change notification settings - Fork 665
CONSOLE-4880: Bump 'yup' version #15737
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
base: main
Are you sure you want to change the base?
Conversation
|
@krishagarwal278: This pull request references CONSOLE-4880 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.21.0" version, but no target version was set. In 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. |
WalkthroughYup schemas across frontend packages were refactored to use functional then-callbacks and schema.concat(), many validation test callbacks were made async, the yup dependency was upgraded, two devconsole locale entries were removed, and several tests and mock data structures were adjusted to match validation changes. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
|
@krishagarwal278: This pull request references CONSOLE-4880 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.21.0" version, but no target version was set. In 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. |
1 similar comment
|
@krishagarwal278: This pull request references CONSOLE-4880 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.21.0" version, but no target version was set. In 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. |
|
@krishagarwal278: This pull request references CONSOLE-4880 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.21.0" version, but no target version was set. In 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 review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-validation-utils.spec.ts (1)
10-25: Test mock data structure does not match validation schema; "optionality" error type does not existThe validation schema expects nested
subject { name, kind }but the test mock data provides a flatuserfield. The schema has no custom test named"optionality"—all three fields (subject.name,subject.kind,role) use only.required(), which produces error type"required", not"optionality".Test 1 modifies a field (
user) that the schema doesn't validate and expects an error type that doesn't exist in the schema definition. Test 2 correctly matches the schema structure. The mock data and/or schema definitions need to be aligned.
🧹 Nitpick comments (7)
frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/__tests__/form-switcher-validation.spec.ts (1)
107-107: Consider using Partial for type safety.The mock error objects use type assertions to bypass TypeScript checking. While this works for tests that only consume the
innerproperty, consider usingPartial<ValidationError>or a mock factory to improve type safety and test maintainability.Example refactor:
- const error = { inner: [{ path: 'spec.tasks[0].name' }] } as ValidationError; + const error = { inner: [{ path: 'spec.tasks[0].name' }] } as Partial<ValidationError>;Also applies to: 123-123, 141-141, 164-164
frontend/packages/knative-plugin/src/components/add/brokers/broker-validation-utils.ts (1)
10-23: Yupwhen(...).thencallback usage is correct; consider addressing the Biome false positiveUsing
then: (schema) => schema.shape({ project, application, name })here is the right pattern for Yup 1.x: it preserves any existing constraints onformDatawhile adding the broker‑specific fields. The BiomenoThenPropertywarning is a generic rule about thenables and doesn’t apply to Yup’swhenconfiguration object, so you may want to suppress this rule for these cases (via config or a targeted// biome-ignore lint/suspicious/noThenPropertycomment) to keep lint noise down.frontend/packages/dev-console/src/components/health-checks/health-checks-probe-validation-utils.ts (1)
16-67: Yup 1.xwhen/thenmigration looks correct; BiomenoThenPropertyis a false positiveThe switch to
then: (schema) => schema.shape({...})fordata,httpGet,tcpSocket, andexecis the right additive pattern for Yup 1.x and keeps the existing shape/constraints intact whenshowForm/requestTypematch.Biome’s
lint/suspicious/noThenPropertyerrors on these lines are false positives for Yup’s.when()options object. Consider either:
- Adding a local ignore (for example,
// biome-ignore lint/suspicious/noThenPropertyimmediately above eachwhen({ is, then: ... })), or- Relaxing this rule in your Biome config for Yup schemas,
so the linter doesn’t keep flagging valid uses of
thenhere.frontend/packages/knative-plugin/src/components/add/eventSource-validation-utils.ts (1)
22-35: Yup conditional schemas are correctly migrated; consider taming Biome’snoThenPropertylintThe refactor to use functional
thencallbacks (then: (schema) => schema.shape(...)/schema.concat(...)) acrosssinkServiceSchema,sourceDataSpecSchema,eventSourceValidationSchema, andaddChannelValidationSchemais consistent with Yup 1.x best practices and keeps the existing validation behavior, while making the conditions additive instead of replacing base schemas.Biome’s
lint/suspicious/noThenPropertyerrors on thesewhen({ is, then })objects are expected with Yup and not an actual bug. It’s worth either:
- Adding targeted ignores around these
whenblocks, or- Adjusting Biome configuration so Yup
.when()options are exempt from this rule,to avoid linter noise on perfectly valid schema definitions.
Also applies to: 37-160, 162-177, 179-194
frontend/packages/dev-console/src/components/import/validation-schema.ts (2)
48-49: Yupwhen/thenrefactors look good; BiomenoThenPropertyremains a lint/config issueAcross
applicationNameValidationSchema,serverlessValidationSchema,routeValidationSchema,imageValidationSchema,gitValidationSchema,dockerValidationSchema,devfileValidationSchema,isiValidationSchema, andimportFlowPipelineTemplateValidationSchema, the migration tothen: (schema) => schema.shape(...)orthen: (schema) => schema.required(...)correctly follows Yup 1.x’s additive pattern and should preserve existing validation behavior.The Biome
lint/suspicious/noThenPropertyerrors on these lines stem from the linter not recognizing Yup’s.when()options signature. You don’t need to change the schema logic; instead, consider either adding localized ignores around thesewhenobjects or updating Biome config so Yup’sthen/otherwisecallbacks aren’t flagged.Also applies to: 80-181, 183-192, 298-307, 309-324, 325-335, 337-351, 359-367, 369-379
214-295: Guard cross-field limit/request tests to avoid convertingundefinedvaluesThe new
.nullable()handling and explicitlimit !== undefined && limit !== nullchecks inlimitsValidationSchemaimprove robustness when limits are left blank.One remaining edge case: in the CPU and memory tests you still call
convertToBaseValue(\${request}${requestUnit}`)(or the symmetric expression) even whenrequestisundefinedbutlimitis set. Depending on howconvertToBaseValuebehaves, that can lead toNaN` comparisons or surprising validation failures when the user leaves one side empty.Consider tightening the guards in both CPU and memory tests, e.g.:
- Only run the comparison when both
requestandlimitare notundefined/null, or- Early-return
truewhen the side being read (requestin the limit test,limitin the request test) isundefined/null.That keeps the cross-field constraints focused on cases where both values are actually present.
frontend/packages/knative-plugin/src/components/sink-source/SinkSource.tsx (1)
78-85: Correct use ofschema.concatfor URI validation; Biome lint is non-blockingSwitching the
sinkwhenbranch to:then: (schema) => schema.concat(sinkTypeUriValidation(t)),is a good improvement — it augments the existing
sinkschema with URI-specific rules without discarding other fields onsink.Biome’s
lint/suspicious/noThenPropertyon thiswhenblock is just the linter disliking athenproperty on plain objects; it’s required by Yup’s API. You can safely keep this code and address the lint via a local ignore or Biome config change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
⛔ Files ignored due to path filters (1)
frontend/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (22)
frontend/package.json(1 hunks)frontend/packages/dev-console/locales/en/devconsole.json(0 hunks)frontend/packages/dev-console/src/components/buildconfig/form-utils/validation.ts(3 hunks)frontend/packages/dev-console/src/components/deployments/utils/deployment-validation-utils.ts(1 hunks)frontend/packages/dev-console/src/components/health-checks/health-checks-probe-validation-utils.ts(1 hunks)frontend/packages/dev-console/src/components/hpa/validation-utils.ts(1 hunks)frontend/packages/dev-console/src/components/import/validation-schema.ts(10 hunks)frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-validation-utils.spec.ts(1 hunks)frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-validation-utils.ts(1 hunks)frontend/packages/knative-plugin/src/components/add/brokers/broker-validation-utils.ts(1 hunks)frontend/packages/knative-plugin/src/components/add/eventSink-validation-utils.ts(1 hunks)frontend/packages/knative-plugin/src/components/add/eventSource-validation-utils.ts(2 hunks)frontend/packages/knative-plugin/src/components/sink-source/SinkSource.tsx(1 hunks)frontend/packages/metal3-plugin/src/components/baremetal-hosts/add-baremetal-host/AddBareMetalHost.tsx(1 hunks)frontend/packages/pipelines-plugin/src/components/pipelines/modals/common/validation-utils.ts(3 hunks)frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/__tests__/form-switcher-validation.spec.ts(5 hunks)frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/validation-utils.ts(3 hunks)frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts(2 hunks)frontend/packages/shipwright-plugin/src/components/build-form/validation.ts(1 hunks)frontend/packages/vsphere-plugin/package.json(1 hunks)frontend/packages/webterminal-plugin/src/components/cloud-shell/setup/cloud-shell-setup-utils.ts(1 hunks)frontend/public/components/configmaps/configmap-utils.ts(1 hunks)
💤 Files with no reviewable changes (1)
- frontend/packages/dev-console/locales/en/devconsole.json
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-validation-utils.spec.tsfrontend/packages/knative-plugin/src/components/sink-source/SinkSource.tsxfrontend/packages/webterminal-plugin/src/components/cloud-shell/setup/cloud-shell-setup-utils.tsfrontend/packages/vsphere-plugin/package.jsonfrontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/__tests__/form-switcher-validation.spec.tsfrontend/public/components/configmaps/configmap-utils.tsfrontend/packages/dev-console/src/components/health-checks/health-checks-probe-validation-utils.tsfrontend/package.jsonfrontend/packages/dev-console/src/components/hpa/validation-utils.tsfrontend/packages/knative-plugin/src/components/add/brokers/broker-validation-utils.tsfrontend/packages/pipelines-plugin/src/components/pipelines/modals/common/validation-utils.tsfrontend/packages/knative-plugin/src/components/add/eventSource-validation-utils.tsfrontend/packages/knative-plugin/src/components/add/eventSink-validation-utils.tsfrontend/packages/dev-console/src/components/deployments/utils/deployment-validation-utils.tsfrontend/packages/dev-console/src/components/import/validation-schema.tsfrontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-validation-utils.tsfrontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/validation-utils.tsfrontend/packages/dev-console/src/components/buildconfig/form-utils/validation.tsfrontend/packages/shipwright-plugin/src/components/build-form/validation.tsfrontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.tsfrontend/packages/metal3-plugin/src/components/baremetal-hosts/add-baremetal-host/AddBareMetalHost.tsx
🪛 Biome (2.1.2)
frontend/packages/knative-plugin/src/components/sink-source/SinkSource.tsx
[error] 82-82: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/webterminal-plugin/src/components/cloud-shell/setup/cloud-shell-setup-utils.ts
[error] 23-23: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/public/components/configmaps/configmap-utils.ts
[error] 208-208: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 212-212: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/dev-console/src/components/health-checks/health-checks-probe-validation-utils.ts
[error] 16-16: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 45-45: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 56-56: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 63-63: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/dev-console/src/components/hpa/validation-utils.ts
[error] 12-12: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/knative-plugin/src/components/add/brokers/broker-validation-utils.ts
[error] 15-15: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/pipelines-plugin/src/components/pipelines/modals/common/validation-utils.ts
[error] 14-14: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 22-22: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 29-29: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 38-38: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 49-49: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 74-74: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 89-89: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 104-104: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 113-113: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/knative-plugin/src/components/add/eventSource-validation-utils.ts
[error] 27-27: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 34-34: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 42-42: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 55-55: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 80-80: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 97-97: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 146-146: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 167-167: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 184-184: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/knative-plugin/src/components/add/eventSink-validation-utils.ts
[error] 18-18: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 25-25: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 33-33: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 50-50: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 58-58: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/dev-console/src/components/deployments/utils/deployment-validation-utils.ts
[error] 116-116: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 125-125: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 140-140: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/dev-console/src/components/import/validation-schema.ts
[error] 48-48: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 80-80: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 188-188: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 301-301: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 317-317: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 328-328: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 340-340: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 375-375: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-validation-utils.ts
[error] 41-41: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/validation-utils.ts
[error] 485-485: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/dev-console/src/components/buildconfig/form-utils/validation.ts
[error] 17-17: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 28-28: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 38-38: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 49-49: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 53-53: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 95-95: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 99-99: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/shipwright-plugin/src/components/build-form/validation.ts
[error] 54-54: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 58-58: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts
[error] 48-48: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 82-82: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 100-100: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 116-116: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (27)
frontend/packages/webterminal-plugin/src/components/cloud-shell/setup/cloud-shell-setup-utils.ts (1)
21-30: LGTM! Yup 1.x conditional migration is correct.The migration from static
thenclause to functional callback pattern is correct for Yup 1.x. The validation logic remains unchanged—when the namespace equalsCREATE_NAMESPACE_KEY, the schema applies the regex match and required validation.Note: The Biome static analysis warning on line 23 (
lint/suspicious/noThenProperty) is a false positive. Thethenproperty is valid in Yup's conditional schema API (yup.when()), and the functional callback patternthen: (schema) => ...is the correct approach for Yup 1.x.frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/__tests__/form-switcher-validation.spec.ts (1)
2-2: LGTM: ValidationError import added for test typing.The import is correctly added to support type assertions in test mocks.
frontend/packages/helm-plugin/src/components/forms/HelmChartRepository/helmchartrepository-validation-utils.ts (1)
35-47: Yup 1.x migration looks correct.The migration correctly implements Yup 1.x patterns:
- Async test function for Promise-based validation
- Functional
thencallback withschema.concat()for conditional schema composition- Explicit
awaitand return for async flowThe Biome lint warning on line 41 is a false positive—
thenis a valid property in Yup's.when()conditional API, not a Promise-like object.frontend/packages/metal3-plugin/src/components/baremetal-hosts/add-baremetal-host/AddBareMetalHost.tsx (1)
135-141: LGTM! Type safety improvement.The migration from
Yup.mixed()toYup.string()appropriately tightens the type for the name field, and the added type annotation(value: string)in the test callback improves TypeScript clarity. The unique-name validation logic remains correct, and the schema composition viaconcat()is compatible with Yup 1.x.frontend/packages/pipelines-plugin/src/components/pipelines/modals/common/validation-utils.ts (5)
7-55: LGTM! Yup 1.x migration correctly applied.The conversion from object-literal
thento function-basedthen: (schema) => schema.shape(...)is correct for Yup 1.x. All conditional validations for git, image, storage, and cluster resource types maintain the same validation requirements.
57-67: LGTM! Schema composition withconcat()correctly implemented.The functional
thenpattern combined withschema.concat(validateResourceType())is the correct Yup 1.x approach for composing validation schemas conditionally.
69-130: LGTM! Volume type validations correctly migrated.All four volume type branches (Secret, ConfigMap, PVC, VolumeClaimTemplate) use the correct Yup 1.x functional
thenpattern. The deeply nested VolumeClaimTemplate schema structure is properly maintained with correct nesting levels.
132-155: LGTM! Common pipeline schema correctly structured.The schema composition is correct, and the synchronous
.test()validation at lines 141-145 is valid for Yup 1.x (no async conversion needed since it returns a boolean without async operations).
1-185: Static analysis warnings are false positives.Biome's
noThenPropertyrule flags allthenproperties in this file, but these are Yup's conditional validation API (when().then()), not Promise thenables. The functional patternthen: (schema) => schema.shape(...)is the correct Yup 1.x syntax.Consider suppressing this Biome rule for validation files using Yup's
when()API:// biome-ignore lint/suspicious/noThenProperty: Yup's conditional validation API uses 'then' propertyOr configure
.biome.jsonto disable this rule for validation utility files:{ "overrides": [ { "include": ["**/*validation*.ts"], "linter": { "rules": { "suspicious": { "noThenProperty": "off" } } } } ] }frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts (4)
46-60: LGTM! Correct Yup 1.x conditional schema pattern.The migration from static shape to functional
thencallback is correct for Yup 1.x compatibility. The Biome warning on line 48 is a false positive—thenis a legitimate property in Yup'swhen()API, not a Promise chain.
61-73: LGTM! Conditional validation and synchronous tests are correct.The functional
thencallback is properly implemented. The custom.test()functions don't require async conversion since they perform synchronous checks onthis.parentwithout callingvalidate().
95-109: LGTM! Conditional validation correctly migrated.The functional
thencallback and synchronous test functions are properly implemented for Yup 1.x.
114-117: LGTM! Schema composition using concat is correct.The use of
schema.concat(pipelinesAccessTokenValidationSchema(t))correctly merges the validation schemas rather than replacing them, which preserves any existing validations on therepositoryfield.frontend/packages/dev-console/src/components/hpa/validation-utils.ts (2)
12-12: Biome warning is a false positive; ignore it.The static analysis warning "Do not add then to an object" misinterprets the Yup API. Here,
thenis a valid property in the options object passed toyup.when(), not the Promise method being added to a plain object.
12-56: Fix cross-field validation edge case when minReplicas is undefined.The review comment correctly identifies a potential validation bug. Since
minReplicaslacks.required()whilemaxReplicashas it, whenminReplicasis undefined, the test expressionundefined <= maxReplicasevaluates tofalse, causingmaxReplicasvalidation to fail even when the user legitimately omits the field.The suggested guard clause on line 51 is the correct defensive approach. Either add the guard:
function (maxReplicas) { const { minReplicas } = this.parent; + if (minReplicas == null) return true; return minReplicas <= maxReplicas; }Or, if
minReplicasshould be required in all cases, add.required(t(...))on line 27 to matchmaxReplicas. Without one of these changes, the form cannot be submitted whenminReplicasis left empty—a usability and consistency issue.frontend/packages/vsphere-plugin/package.json (1)
9-13: Yup version bump here is consistent with the root package and looks good
@console/vsphere-pluginnow depends onyup@^1.7.1, which keeps it in sync with the main frontend package and avoids mixed major versions of Yup within the workspace. Just ensure vsphere-plugin validation flows/tests are exercised after the upgrade.frontend/package.json (1)
135-221: Root Yup upgrade is scoped and appropriateBumping the main
openshift-consoledependency toyup@^1.7.1aligns with the schema changes in this PR. The change is isolated to the Yup version, so the main follow‑up is to run the recommended validation‑focused test suites to catch any subtle behavioral differences.frontend/packages/shipwright-plugin/src/components/build-form/validation.ts (1)
44-65: Async Yup test withschema.concatis a sound migration patternThe
async test(values)implementation that builds aformYamlDefinitionschema, awaitsformYamlDefinition.validate(values, { abortEarly: false }), and returnstrueis a solid adaptation for Yup 1.x. Usingthen: (schema) => schema.concat(formDataSchema())/schema.concat(yup.string().required(...))ensures editor‑type‑specific constraints are layered on top of the base schema instead of replacing it. Any validation failures will still propagate via the rejectedvalidatecall.frontend/public/components/configmaps/configmap-utils.ts (1)
191-219: ConfigMap form/YAML validation migration aligns with the new Yup patternThe updated
validationSchemacorrectly switches to anasync test(values)that delegates to aformYamlDefinitionschema and returnstrueon successful validation. Usingwhen('editorType', { then: (schema) => schema.concat(formDataSchema(values)) })and a similar pattern foryamlDatacleanly composes editor‑specific constraints with the base schema while still allowingkeyValueValidation(values)to enforce uniqueness acrossdataandbinaryData.frontend/packages/pipelines-plugin/src/components/pipelines/pipeline-builder/validation-utils.ts (1)
248-305: Resource/workspace checks and async form/YAML validation look correctCoercing
resourceValueandworkspaceValuewith!!in the required tests makes the intent explicit without changing behavior, since these fields are string identifiers. The updatedvalidationSchemathat usesasync test(formValues)pluswhen('editorType', { then: (schema) => schema.concat(pipelineBuilderFormSchema(formValues)) })and awaitsformYamlDefinition.validate(...)is consistent with the other Yup 1.x migrations and should continue to enforce the same structural constraints on pipeline builder forms.Also applies to: 383-395, 477-491
frontend/packages/dev-console/src/components/deployments/utils/deployment-validation-utils.ts (1)
106-130: Deployment edit validation uses the right Yup 1.x composition patternsThe
imageStreamandisibranches now usethen: (schema) => schema.shape({ ... }), which extends the existing object schema instead of replacing it, and the top-levelvalidationSchemawraps the combined Form/YAML validation in anasync test(formValues)that awaitsformYamlDefinition.validate(formValues, { abortEarly: false })and returnstrue. This matches the other updated validators in the PR and keeps the conditionalfromImageStreamTagbehavior intact while aligning with Yup 1.x’s Promise-based API.Also applies to: 132-146
frontend/packages/dev-console/src/components/buildconfig/form-utils/validation.ts (2)
33-55: Async top-level test correctly adapts to Yup 1.x; ensure all callers handle the PromiseThe updated
validationSchemanow uses anasync testandawait formYamlDefinition.validate(values, { abortEarly: false }), which is the right way to migrate away fromvalidateSyncin Yup 1.x while preserving aggregated error reporting.Since the test is now asynchronous, callers must treat
validationSchema()as returning a schema whosevalidatemethod yields aPromise. This matches how Formik and your existing validation plumbing typically integrate with Yup, but please confirm there are no call sites expecting purely synchronous validation for this schema.Also applies to: 85-105
15-31: Verify that thedockerfilefield validation is intentionally optionalThe async test pattern is correct for Yup 1.x:
await formYamlDefinition.validate(values, { abortEarly: false })properly handles asynchronous validation and will throw detailed errors as expected.However, the
dockerfilebranch shows an asymmetry worth confirming:dockerfile: yup.string().when('type', { is: 'dockerfile', then: (schema) => schema, // no-op: zero constraints applied }),Unlike the
gitbranch—which applies.shape()with a requiredurlfield whentype === 'git'—thedockerfilefield remains unconstrained whentype === 'dockerfile'. If the form should require Dockerfile content/path in that mode, add.required(i18n.t('devconsole~Required'))to thethencallback. If it is intentionally optional, document that expectation or confirm with your test suite.frontend/packages/knative-plugin/src/components/add/eventSink-validation-utils.ts (4)
13-26: LGTM: Correct Yup 1.x migration pattern.The conditional validation logic has been correctly migrated to Yup 1.x's functional callback pattern. The use of
schema.concat()on line 25 properly composes the URI validation schema.
48-51: Verify the no-op conditional branch.The
CamelKameletBindingModel.kindbranch returns the schema unchanged (then: (schema) => schema). Please confirm whether this is intentional (to explicitly document that this type requires no additional validation) or if this conditional branch can be removed.
53-68: LGTM: Correct schema composition.The form validation logic has been correctly migrated to use the functional
thencallback, properly composing all nested validation schemas (project, application, name, source, data).
13-68: Address Biome linter false positives.The Biome linter is flagging
lint/suspicious/noThenPropertyerrors on lines 18, 25, 33, 50, and 58. These are false positives—thethenproperty is part of Yup's conditional validation API (.when()configuration object) and is not related to Promise chaining.Consider updating the Biome configuration to exclude this rule for Yup validation schema files, or add inline suppressions to prevent confusion:
// biome-ignore lint/suspicious/noThenProperty: Yup DSL requires 'then' property then: (schema) => schema.shape({ ... })
frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts (1)
112-119: Schema concatenation is correct.The use of
schema.concat()on line 116 correctly merges the webhook validation schema, following the Yup 1.x composition pattern.The webhook validation logic in
repositoryValidationSchema(lines 44-73) andpipelinesAccessTokenValidationSchema(lines 78-109) is duplicated. Consider extracting the webhook validation into a shared helper function to reduce duplication and maintenance burden:const webhookValidationShape = (t: TFunction) => ({ webhook: yup .object() .when('gitProvider', { is: GitProvider.BITBUCKET, then: (schema) => schema.shape({ user: yup.string() .matches(bitBucketUserNameRegex, { message: t('pipelines-plugin~Name must consist of lower-case letters, numbers, underscores and hyphens. It must start with a letter and end with a letter or number.'), excludeEmptyString: true, }) .required(t('pipelines-plugin~Required')), }), }) .when(['method', 'gitProvider', 'gitUrl'], { is: (method, gitProvider, gitUrl) => gitUrl && !(gitProvider === GitProvider.GITHUB && method === GitProvider.GITHUB), then: (schema) => schema.shape({ token: yup.string().test('oneOfRequired', 'Required', function () { return this.parent.token || this.parent.secretRef; }), secretRef: yup.string().test('oneOfRequired', 'Required', function () { return this.parent.token || this.parent.secretRef; }), }), }), });Then use it in both schemas:
export const repositoryValidationSchema = (t: TFunction) => yup.object().shape({ name: yup.string()..., gitUrl: yup.string()..., accessToken: yup.string(), ...webhookValidationShape(t), });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (3)
frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-data.ts(1 hunks)frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-validation-utils.spec.ts(1 hunks)frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-data.tsfrontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-validation-utils.spec.tsfrontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts
🪛 Biome (2.1.2)
frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts
[error] 48-48: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 64-64: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 82-82: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 100-100: Do not add then to an object.
(lint/suspicious/noThenProperty)
[error] 116-116: Do not add then to an object.
(lint/suspicious/noThenProperty)
🔇 Additional comments (4)
frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-validation-utils.spec.ts (1)
8-8: LGTM! Test correctly updated for new data structure.The test properly reflects the updated mock data structure, accessing the name field via the nested
subjectobject. The test logic and validation assertions remain intact.frontend/packages/dev-console/src/components/project-access/__tests__/project-access-form-data.ts (1)
6-9: Data structure change verified and approved.Verification confirms the validation schema in
project-access-form-validation-utils.ts(lines 7–10) expects the exact nestedsubjectstructure introduced in your change. The test data structure at lines 6–9 aligns perfectly with the schema requirements.frontend/packages/pipelines-plugin/src/components/repository/repository-form-utils.ts (2)
28-110: Yup 1.x migration looks correct.The conversion from object-based
thento functional callbacks(schema) => schema.shape(...)correctly follows the Yup 1.x API. The synchronous test functions (lines 66-71, 102-107) are valid since they only check properties without async operations. The previous issue withnameRegexvsbitBucketUserNameRegexhas been properly addressed.
48-48: Biome warnings are false positives.The static analysis warnings about "Do not add then to an object" are false positives. These lines correctly use Yup 1.x's conditional validation API where
thenis a property that takes a function(schema) => .... This is not the Promise anti-pattern that Biome'snoThenPropertyrule is designed to catch.Consider suppressing these warnings or configuring Biome to ignore
thenproperties withinyup.when()calls.Also applies to: 64-64, 82-82, 100-100, 116-116
|
/label px-approved |
|
/lgtm |
|
/verified by @krishagarwal278 |
|
@krishagarwal278: This PR has been marked as verified by In 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. |
jhadvig
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, krishagarwal278, Leo6Leo The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest-required |
|
New changes are detected. LGTM label has been removed. |
|
/retest |
a1dfbec to
1f6b216
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
frontend/packages/dev-console/src/components/import/deployImage-validation-utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/dev-console/src/components/import/deployImage-validation-utils.ts
🪛 Biome (2.1.2)
frontend/packages/dev-console/src/components/import/deployImage-validation-utils.ts
[error] 23-23: Do not add then to an object.
(lint/suspicious/noThenProperty)
frontend/packages/dev-console/src/components/import/deployImage-validation-utils.ts
Outdated
Show resolved
Hide resolved
|
@krishagarwal278: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/retest |
Bump Yup from 0.27.0 to 1.7.1
Summary
Upgrades Yup validation library from 0.27.0 to 1.7.1, bringing native async validation support, improved type safety, and smaller bundle size (10.26 KB gzipped). Migrated 20+ validation files to comply with Yup 1.x breaking changes.
Key Changes
yup.mixed().test()functions to useasync/await(validate() now always returns Promise)yup.when().then()clauses to function-based pattern:(schema) => schema.concat(...)orschema.shape({...})yup@^1.7.1infrontend/package.jsonandfrontend/packages/vsphere-plugin/package.jsonMigration Pattern
Before:
After:
Files Modified
Core: buildconfig, deployments, health-checks, hpa, import validation schemas
Plugins: helm, knative, pipelines, shipwright, webterminal, metal3 validations
Public: configmaps validation
Tests: form-switcher-validation.spec.ts
Why This Matters
Testing
yarn test --testPathPattern="validation"and verify form validations in Pipeline builder, Build config, Deployment, ConfigMap, Helm, and Knative formsAdditional Info
Breaking Changes Addressed:
yup/lib/*imports)Risk Level: 🟡 Moderate - Validation logic preserved, straightforward rollback if needed
This PR Description is generated by Claude Code