chore | Hide forkliftController hidden fields in the installer automated UI form.#3204
Merged
yaacov merged 1 commit intokubev2v:mainfrom Oct 22, 2025
Conversation
yaacov
requested review from
Hazanel,
mnecas,
mrnold and
solenoci
as code owners
WalkthroughForkliftController CRD updates across multiple manifest directories replace string pattern validations with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The diff exhibits consistent refactoring patterns across multiple manifest files (same field conversions repeated uniformly), reducing per-file cognitive load. However, the scope is extensive—20+ field updates per manifest across 6+ YAML files plus tooling changes. CRD schema changes warrant careful verification of type compatibility, unknown-field handling, and specDescriptor alignment across all manifest variants (downstream, kustomized, upstream, base definitions). Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
yaacov
force-pushed
the
forklift-controller-crd-allow-unknown-types-for-feature-flags
branch
from
ccc1665 to
a276458
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3204 +/- ##
=========================================
- Coverage 15.45% 8.35% -7.10%
=========================================
Files 112 391 +279
Lines 23377 53760 +30383
=========================================
+ Hits 3613 4493 +880
- Misses 19479 48916 +29437
- Partials 285 351 +66
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
operator/.downstream_manifests(5 hunks)operator/.kustomized_manifests(5 hunks)operator/.upstream_manifests(5 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
📚 Learning: 2025-09-18T13:48:36.037Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-01T14:02:10.056Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-01T16:36:42.418Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-03T07:08:53.741Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifestsoperator/.downstream_manifests
📚 Learning: 2025-10-16T01:41:12.346Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.kustomized_manifests:304-323
Timestamp: 2025-10-16T01:41:12.346Z
Learning: In Forklift operator CRD/manifests (e.g., operator/.kustomized_manifests and related bases/downstream files), maintainers prefer field descriptions that add explanatory detail rather than repeating the field name verbatim (e.g., for ova_proxy_fqin, describe the proxy’s purpose and behavior).
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifests
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-18T19:05:05.746Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-10-15T22:30:05.286Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.downstream_manifests:311-317
Timestamp: 2025-10-15T22:30:05.286Z
Learning: In operator manifests (e.g., operator/.downstream_manifests) for ForkliftController CRD fields, maintainers prefer that example values do not need to match the documented defaults; examples can illustrate non-default configurations (e.g., ova_proxy_container_requests_cpu and ova_proxy_container_requests_memory examples differ from their defaults).
Applied to files:
operator/.kustomized_manifestsoperator/.downstream_manifests
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (5)
operator/.upstream_manifests (1)
131-197: Structural schema break:x-kubernetes-preserve-unknown-fieldson scalar fields will be rejectedYou added
x-kubernetes-preserve-unknown-fields: trueto scalar properties and removed theirtype. This is only valid for object-typed schemas. Without atype(or with non-object types), the CRD becomes non‑structural and will be rejected by the apiserver (minKubeVersion 1.27), and OLM scorecard will likely fail.OpenAPI for CRDs cannot express “string OR boolean” unions, except the special IntOrString case. To safely achieve the PR goal, consider one of these options:
- Remove these properties from
spec.propertiesentirely and rely onspec.additionalProperties: true(repo practice) so the fields are untyped and accept bools/strings. Keep docs/examples elsewhere. Minimal CRD impact. Based on learnings.- Keep them as
type: stringand document that values must be quoted; update operator to coerce if needed (does not accept raw booleans at API level).- Switch to
type: booleanand update defaults/samples/operator (breaking for existing string users unless a conversion webhook is added).- Longer‑term refactor: group feature flags under a
features: {}map (type: object,x-kubernetes-preserve-unknown-fields: true) and stop defining individual scalar properties.To confirm the constraint and locate other occurrences:
Run this in the repo to review contexts where
x-kubernetes-preserve-unknown-fields: trueis used without an adjacenttype: object:Also applies to: 235-237, 362-365
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (1)
47-47: Relaxed typing via x-kubernetes-preserve-unknown-fields looks good; please verify operator handling and manifest consistency.This change aligns with the project’s “hinting, not strict validation” CRD philosophy and should accept both booleans and strings as intended. Please:
- Ensure the operator normalizes these values (bool and string "true"/"false") and does not rely on string-only comparisons.
- Check other manifests for any remaining "true"/"false" enum constraints for these fields.
Run these checks:
Optionally, to retain minimal validation while still accepting both types, consider anyOf:
- oneOf/anyOf with types boolean or string (enum ["true","false"]). If you prefer no validation in CRD, current approach is consistent with prior decisions. Based on learnings.
Also applies to: 50-50, 53-53, 56-56, 59-59, 62-62, 65-65, 68-68, 71-71, 288-288, 291-291, 294-294, 297-297, 328-328, 402-402
operator/.downstream_manifests (1)
133-133: Leaf fields using x-kubernetes-preserve-unknown-fields without a type may break structural schema; verify and consider a union (boolean|string).You replaced string enums with x-kubernetes-preserve-unknown-fields: true on multiple spec fields, but left them untyped. In CRD v1 structural schemas, leaf schemas typically must declare a type; preserve-unknown-fields is intended for objects. This may cause CRD validation failures.
If structural validation fails, prefer a union schema that accepts both booleans and strings:
- Minimal: anyOf: [{type: boolean}, {type: string}] (keeps loose typing)
- Stricter: anyOf: [{type: boolean}, {type: string, enum: ["true","false"]}]
Example (apply similarly to all affected fields):
- x-kubernetes-preserve-unknown-fields: true + anyOf: + - type: boolean + - type: stringFields affected here:
- controller_ovirt_warm_migration (Line 133)
- controller_retain_precopy_importer_pods (Line 140)
- controller_static_udn_ip_addresses (Line 155)
- controller_vsphere_incremental_backup (Line 166)
- feature_auth_required (Line 169)
- feature_cli_download (Line 172)
- feature_copy_offload (Line 175)
- feature_ocp_live_migration (Line 178)
- feature_ova_appliance_management (Line 182)
- feature_ui_plugin (Line 185)
- feature_validation (Line 188)
- feature_vmware_system_serial_number (Line 191)
- feature_volume_populator (Line 194)
- k8s_cluster (Line 237)
- virt_v2v_dont_request_kvm (Line 364)
Based on learnings
Run to detect preserve-unknown-at-leaf without a type in ForkliftController CRD:
Also applies to: 140-140, 155-155, 166-166, 169-169, 172-172, 175-175, 178-178, 182-182, 185-185, 188-188, 191-191, 194-194, 237-237, 364-364
operator/.kustomized_manifests (2)
6763-6772: Ensure runtime coercion to strings where required (envs/templates).Now that CRD accepts booleans, verify operator/Ansible templates coerce these values to strings where the target API requires string (e.g., Deployment env.value). Add explicit string casting (e.g., Jinja
| string) if missing.Also applies to: 6903-6917
131-134: Structural schema break:x-kubernetes-preserve-unknown-fieldsmisused on leaf fields; switch toanyOf: [string|boolean].On the listed fields,
typewas removed andx-kubernetes-preserve-unknown-fields: truewas added. That extension is intended for object-typed nodes; on scalars it causes non‑structural schema and is likely rejected by the API server. To accept both booleans and strings while keeping the CRD structural, replace withanyOf:
- Keeps the permissive philosophy while remaining valid
- Optionally constrain the string branch to "true"/"false"
Apply this pattern to all affected flags (controller_ovirt_warm_migration, controller_retain_precopy_importer_pods, controller_static_udn_ip_addresses, controller_vsphere_incremental_backup, feature_* set, k8s_cluster, virt_v2v_dont_request_kvm).
Example fix (apply to each field listed above):
- controller_ovirt_warm_migration: - description: 'Enable oVirt warm migration (default: true)' - x-kubernetes-preserve-unknown-fields: true + controller_ovirt_warm_migration: + description: 'Enable oVirt warm migration (default: true)' + anyOf: + - type: string + - type: booleanOptional (if you still want to hint allowed string values):
- anyOf: - - type: string - - type: boolean + anyOf: + - type: boolean + - type: string + enum: ["true","false"]Please replicate for:
- Lines 131-134, 138-141, 153-156, 164-167, 167-170, 170-173, 173-176, 176-179, 180-183, 183-186, 186-189, 189-192, 192-195, 234-238, 362-365.
This keeps the CRD structural and achieves the PR goal. Based on learnings.
Run to locate any remaining leaf usages of
x-kubernetes-preserve-unknown-fieldsand ensureanyOfis used instead:Also applies to: 138-141, 153-156, 164-167, 167-170, 170-173, 173-176, 176-179, 180-183, 183-186, 186-189, 189-192, 192-195, 234-238, 362-365
yaacov
force-pushed
the
forklift-controller-crd-allow-unknown-types-for-feature-flags
branch
4 times, most recently
from
393b1e3 to
4e4ce58
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/.kustomized_manifests (1)
83-151: Add missingtype: stringto allx-kubernetes-int-or-stringandx-kubernetes-preserve-unknown-fieldsfields
- Numeric fields (
controller_block_overhead,controller_cleanup_retries, …validation_policy_agent_search_interval) withx-kubernetes-int-or-string: true- Boolean toggles (
controller_ovirt_warm_migration,controller_retain_precopy_importer_pods,controller_static_udn_ip_addresses) withx-kubernetes-preserve-unknown-fields: trueApply in both:
- operator/.kustomized_manifests
- operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
Ensures a structural schema and prevents validation/pruning failures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
operator/.downstream_manifests(5 hunks)operator/.kustomized_manifests(5 hunks)operator/.upstream_manifests(5 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(4 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 Common learnings
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.downstream_manifests:311-317
Timestamp: 2025-10-15T22:30:05.286Z
Learning: In operator manifests (e.g., operator/.downstream_manifests) for ForkliftController CRD fields, maintainers prefer that example values do not need to match the documented defaults; examples can illustrate non-default configurations (e.g., ova_proxy_container_requests_cpu and ova_proxy_container_requests_memory examples differ from their defaults).
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.downstream_manifests:2523-2534
Timestamp: 2025-09-03T04:08:05.745Z
Learning: For Forklift Plan CRD docs, maintainer prefers documenting existing runtime keys/behavior verbatim (e.g., TargetLabels uses "app", convertor pods use "forklift.app") and avoiding normalization changes in documentation-only PRs.
📚 Learning: 2025-10-16T13:20:28.803Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Applied to files:
operator/.downstream_manifestsoperator/.upstream_manifestsoperator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-16T12:04:39.638Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:1253-1270
Timestamp: 2025-09-16T12:04:39.638Z
Learning: The files operator/.kustomized_manifests and operator/.upstream_manifests are also auto-generated manifests in the Forklift project, similar to operator/.downstream_manifests.
Applied to files:
operator/.upstream_manifests
📚 Learning: 2025-09-18T13:48:36.037Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-10-15T22:30:05.286Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.downstream_manifests:311-317
Timestamp: 2025-10-15T22:30:05.286Z
Learning: In operator manifests (e.g., operator/.downstream_manifests) for ForkliftController CRD fields, maintainers prefer that example values do not need to match the documented defaults; examples can illustrate non-default configurations (e.g., ova_proxy_container_requests_cpu and ova_proxy_container_requests_memory examples differ from their defaults).
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifests
📚 Learning: 2025-10-16T01:41:12.346Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.kustomized_manifests:304-323
Timestamp: 2025-10-16T01:41:12.346Z
Learning: In Forklift operator CRD/manifests (e.g., operator/.kustomized_manifests and related bases/downstream files), maintainers prefer field descriptions that add explanatory detail rather than repeating the field name verbatim (e.g., for ova_proxy_fqin, describe the proxy’s purpose and behavior).
Applied to files:
operator/.upstream_manifestsoperator/.kustomized_manifests
📚 Learning: 2025-09-18T19:05:05.746Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Applied to files:
operator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-01T16:36:42.418Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Applied to files:
operator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-01T14:02:10.056Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Applied to files:
operator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-03T07:08:53.741Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
Applied to files:
operator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-03T04:08:05.745Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.downstream_manifests:2523-2534
Timestamp: 2025-09-03T04:08:05.745Z
Learning: For Forklift Plan CRD docs, maintainer prefers documenting existing runtime keys/behavior verbatim (e.g., TargetLabels uses "app", convertor pods use "forklift.app") and avoiding normalization changes in documentation-only PRs.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-08-18T13:45:08.364Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:694-699
Timestamp: 2025-08-18T13:45:08.364Z
Learning: The user yaacov prefers not to change existing behavior within the scope of a current PR when additional validation or safety checks are identified, and instead prefers to create follow-up issues to address such improvements separately.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-09-01T16:34:51.909Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:233-236
Timestamp: 2025-09-01T16:34:51.909Z
Learning: The maintainer yaacov prefers to keep PRs focused on their intended scope and handle different types of changes (like typo fixes vs documentation updates) in separate issues/PRs. When documenting existing APIs, the documentation should accurately reflect the current implementation state, even if it contains typos that will be fixed later.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (6)
operator/.upstream_manifests (3)
6755-6758: Examples keep canonical string flags — goodRetaining string "true"/"false" in sample CR and CSV init matches the guidance to prefer string forms while temporarily accepting booleans. Based on learnings
Also applies to: 6898-6901
126-126: **** The suggestion contradicts the established repo preference documented in the learnings from PR #3204. Maintainers explicitly prefer not to document both boolean and string types per field. The current use ofx-kubernetes-preserve-unknown-fields: trueis intentional—a temporary workaround that accepts booleans for backward-compatibility while steering users toward canonical string values ("true"/"false"). Replacing it with ananyOfunion would document both types, which goes against the maintainer's stated guidance. No changes are needed.Likely an incorrect or invalid review comment.
85-85: The schema is valid as-is. Kubernetes explicitly exemptsx-kubernetes-int-or-string: truefrom the structural schema "must specify a type" requirement per official CRD documentation. While addinganyOfis optional and used in some CRDs (OpenShift quota/route), it is not required for structural schema compliance. Additionally, this file is auto-generated, so manual edits would be overwritten on regeneration. No action needed.Likely an incorrect or invalid review comment.
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (2)
47-47: LGTM: CRD now preserves unknown fields on all boolean-like toggles and templates coerce inputs via|bool; keep user guidance toward canonical string "true"/"false" (boolean support is temporary).
36-44: Defaults-CRD consistency check passes; no sync issues found.All CRD schema properties are present in
operator/roles/forkliftcontroller/defaults/main.yml. The 69 extra keys in defaults are internal infrastructure naming variables (e.g.,api_deployment_name,controller_service_name,validation_tls_secret_name) intentionally undocumented in the CRD schema, consistent with the design that the CRD documents only user-overridable configuration knobs.operator/.downstream_manifests (1)
85-85: Add anyOf blocks to all IntOrString fields for structural CRD complianceEach
x-kubernetes-int-or-string: trueentry inoperator/.downstream_manifestsmust include an explicit anyOf before the annotation, for example:controller_cleanup_retries: description: 'Cleanup retry count (default: 10)' anyOf: - type: integer - type: string x-kubernetes-int-or-string: trueApply this pattern to every IntOrString field, then rerun your CRD generation/scorecard to confirm no “non-structural schema” warnings remain.
Member
Author
|
@coderabbitai can you resolve the comments that we aggread apon ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 62140 🏁 Script executed: Length of output: 1339 🏁 Script executed: Length of output: 3245 🏁 Script executed: Length of output: 1100 @yaacov Done! ✅ I've successfully resolved all the review comments we agreed upon: Resolved threads:
All 6 review threads are now marked as resolved on the PR. 🧠 Learnings used |
Member
Author
|
@mansam thanks, i found an issue when using the UI: will add a type in addition to the "unknown" to fix to: it will hing the UI to use booleans and allow k8s to accept strings too: 
|
yaacov
force-pushed
the
forklift-controller-crd-allow-unknown-types-for-feature-flags
branch
3 times, most recently
from
a264617 to
f68d1cf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
operator/.downstream_manifests(6 hunks)operator/.kustomized_manifests(6 hunks)operator/.upstream_manifests(6 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(4 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
📚 Learning: 2025-10-16T14:11:57.015Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.downstream_manifestsoperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:44:49.699Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:20:28.803Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:45:36.153Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:44:44.382Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:46:22.250Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-18T13:48:36.037Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:32:31.284Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:151-153
Timestamp: 2025-09-01T16:32:31.284Z
Learning: The field controller_snapshot_removal_timeout_minuts in ForkliftController CRD contains a typo - it should be controller_snapshot_removal_timeout_minutes. The maintainer yaacov prefers to handle such fixes in separate issues rather than mixing them with documentation PRs.
Applied to files:
operator/.downstream_manifests
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/.downstream_manifestsoperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-03T07:08:53.741Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
Applied to files:
operator/.downstream_manifestsoperator/.kustomized_manifests
📚 Learning: 2025-09-01T16:36:42.418Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Applied to files:
operator/.downstream_manifestsoperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T14:02:10.056Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Applied to files:
operator/.downstream_manifests
📚 Learning: 2025-09-18T19:05:05.746Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml
📚 Learning: 2025-10-16T01:41:12.346Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.kustomized_manifests:304-323
Timestamp: 2025-10-16T01:41:12.346Z
Learning: In Forklift operator CRD/manifests (e.g., operator/.kustomized_manifests and related bases/downstream files), maintainers prefer field descriptions that add explanatory detail rather than repeating the field name verbatim (e.g., for ova_proxy_fqin, describe the proxy’s purpose and behavior).
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-03T04:08:05.745Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.downstream_manifests:2523-2534
Timestamp: 2025-09-03T04:08:05.745Z
Learning: For Forklift Plan CRD docs, maintainer prefers documenting existing runtime keys/behavior verbatim (e.g., TargetLabels uses "app", convertor pods use "forklift.app") and avoiding normalization changes in documentation-only PRs.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-08-18T13:45:08.364Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:694-699
Timestamp: 2025-08-18T13:45:08.364Z
Learning: The user yaacov prefers not to change existing behavior within the scope of a current PR when additional validation or safety checks are identified, and instead prefers to create follow-up issues to address such improvements separately.
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:34:51.909Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:233-236
Timestamp: 2025-09-01T16:34:51.909Z
Learning: The maintainer yaacov prefers to keep PRs focused on their intended scope and handle different types of changes (like typo fixes vs documentation updates) in separate issues/PRs. When documenting existing APIs, the documentation should accurately reflect the current implementation state, even if it contains typos that will be fixed later.
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-15T22:30:05.286Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.downstream_manifests:311-317
Timestamp: 2025-10-15T22:30:05.286Z
Learning: In operator manifests (e.g., operator/.downstream_manifests) for ForkliftController CRD fields, maintainers prefer that example values do not need to match the documented defaults; examples can illustrate non-default configurations (e.g., ova_proxy_container_requests_cpu and ova_proxy_container_requests_memory examples differ from their defaults).
Applied to files:
operator/.upstream_manifests
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: lint
🔇 Additional comments (11)
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (2)
47-48: Boolean + preserve fields are already protected by role-layer|boolfilters—verification confirms the pattern is safe.All role/template usage employs Ansible's
|boolfilter (e.g.,feature_ui_plugin|bool,k8s_cluster|bool), which converts both boolean and string values seamlessly. The implementation aligns with the maintainers' temporary-compatibility approach (per learnings). The|boolfilter ensures that whether the CRD receivestrue(boolean) or"true"(string), the role layer will handle it correctly.Note:
x-kubernetes-preserve-unknown-fieldsalone does not relax schema validation; it is the role-layer filter logic that provides compatibility. Examples throughout the codebase use boolean literals (not strings), so the intended consumption pattern is already in place.
249-249: Do not add explicittype: stringfor int-or-string fields
x-kubernetes-int-or-string: trueis a built-in exception in Kubernetes structural schemas and does not require an accompanyingtype: string.Likely an incorrect or invalid review comment.
operator/.downstream_manifests (3)
85-85: Numeric flags: correct use of x-kubernetes-int-or-stringGood relaxation for counters while keeping structural schema intact. No further changes.
Also applies to: 88-88, 107-107, 110-110, 117-117, 120-120, 123-123, 130-130, 137-137, 140-140, 143-143, 150-150, 153-153, 341-341
126-127: Boolean-like toggles: boolean + preserve-unknown is alignedThis achieves console boolean widgets and tolerates legacy strings. Looks good.
Also applies to: 133-134, 146-147, 156-157, 160-161, 164-165, 168-169, 172-173, 177-178, 181-182, 185-186, 189-190, 193-194, 237-238, 364-365
6913-6916: CSV init resource keeps canonical string "true"Consistent with guidance. LGTM.
operator/.kustomized_manifests (2)
85-85: Adopting x-kubernetes-int-or-string on counters — LGTMMatches repo guidance for numeric unions without oneOf/anyOf.
Also applies to: 88-88, 107-107, 110-110, 117-117, 120-120, 123-123, 130-130, 137-137, 140-140, 143-143, 150-150, 153-153, 341-341
126-127: Boolean type + preserve-unknown on feature/controller flags — LGTMAligns with the temporary back-compat plan and resolves the console UI rendering concern while tolerating string inputs.
Also applies to: 133-134, 146-147, 156-157, 160-161, 164-165, 168-169, 172-173, 177-178, 181-182, 185-186, 189-190, 193-194, 237-238, 364-365
operator/.upstream_manifests (4)
138-141: Typo intentionally retained.controller_snapshot_removal_timeout_minuts keeps the known typo while changing validation only. Acknowledge keeping it as-is in this PR. Based on learnings
Please confirm no accidental field-name changes elsewhere for this key.
6770-6772: Sample CR booleans: good for console UX.Boolean values in the sample reinforce the console UI controls while the schema still permits strings. LGTM. Based on learnings
Confirm CSV “initialization-resource” intentionally keeps canonical string "true"/"false" values.
83-124: Approve int-or-string coverage Verified that all intended numeric and retry fields includex-kubernetes-int-or-string: true. LGTM.
126-127: All boolean toggles include x-kubernetes-preserve-unknown-fields: true aligns with the temporary compatibility strategy; no gaps detected.
Comment on lines
+6770
to
+6772
| feature_ui_plugin: true | ||
| feature_validation: true | ||
| feature_volume_populator: true |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Example CR values: prefer canonical string "true"
To steer users toward the canonical representation while boolean acceptance is temporary, consider quoting these as strings.
-spec:
- feature_ui_plugin: true
- feature_validation: true
- feature_volume_populator: true
+spec:
+ feature_ui_plugin: "true"
+ feature_validation: "true"
+ feature_volume_populator: "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.
Suggested change
| feature_ui_plugin: true | |
| feature_validation: true | |
| feature_volume_populator: true | |
| spec: | |
| feature_ui_plugin: "true" | |
| feature_validation: "true" | |
| feature_volume_populator: "true" |
🤖 Prompt for AI Agents
In operator/.downstream_manifests around lines 6770 to 6772, the manifest uses
bare boolean literals (true) for feature_ui_plugin, feature_validation, and
feature_volume_populator; the reviewer requests the canonical string form.
Update each value to the quoted string "true" (feature_ui_plugin: "true",
feature_validation: "true", feature_volume_populator: "true") so the CR uses the
canonical string representation while boolean acceptance remains temporary.
Member
Author
There was a problem hiding this comment.
long term will be boolean
There was a problem hiding this comment.
@yaacov Understood. I’m withdrawing the nit and will keep the sample values as booleans in operator/.downstream_manifests (around Line 6767). This aligns with the stored repo guidance that samples/docs should prefer boolean true/false going forward.
I also have an older learning that suggested steering users to string "true"/"false" while boolean acceptance was temporary (ID: 497ea3fa-33f4-460d-8f7f-cf6252e05fc4). If that’s now obsolete, confirm and I’ll remove it from my knowledge base.
🧠 Learnings used
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.582Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.downstream_manifests:311-317
Timestamp: 2025-10-15T22:30:05.286Z
Learning: In operator manifests (e.g., operator/.downstream_manifests) for ForkliftController CRD fields, maintainers prefer that example values do not need to match the documented defaults; examples can illustrate non-default configurations (e.g., ova_proxy_container_requests_cpu and ova_proxy_container_requests_memory examples differ from their defaults).
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Member
Author
There was a problem hiding this comment.
yes please remove the learning
There was a problem hiding this comment.
@yaacov Removed. I’ll keep our guidance that samples/docs should use boolean true/false going forward.
Optionally, I also have a related learning (ID: 1d07a6d9) that mentions docs should prefer string "true"/"false" while using preserve-unknown-fields. Should I update/remove that one as well to align fully with the boolean direction?
⛔ Learnings removed
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
🧠 Learnings used
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.393Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.582Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.downstream_manifests:311-317
Timestamp: 2025-10-15T22:30:05.286Z
Learning: In operator manifests (e.g., operator/.downstream_manifests) for ForkliftController CRD fields, maintainers prefer that example values do not need to match the documented defaults; examples can illustrate non-default configurations (e.g., ova_proxy_container_requests_cpu and ova_proxy_container_requests_memory examples differ from their defaults).
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Comment on lines
+6769
to
+6772
| spec: | ||
| feature_ui_plugin: "true" | ||
| feature_validation: "true" | ||
| feature_volume_populator: "true" | ||
| feature_ui_plugin: true | ||
| feature_validation: true | ||
| feature_volume_populator: true |
There was a problem hiding this comment.
Example CR uses booleans; prefer canonical strings to avoid advertising the fallback
Per prior decision, keep user-facing samples using "true"/"false" strings. Switch the example to strings for consistency with the CSV init resource.
Based on learnings
spec:
- feature_ui_plugin: true
- feature_validation: true
- feature_volume_populator: true
+ feature_ui_plugin: "true"
+ feature_validation: "true"
+ feature_volume_populator: "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.
Suggested change
| spec: | |
| feature_ui_plugin: "true" | |
| feature_validation: "true" | |
| feature_volume_populator: "true" | |
| feature_ui_plugin: true | |
| feature_validation: true | |
| feature_volume_populator: true | |
| spec: | |
| feature_ui_plugin: "true" | |
| feature_validation: "true" | |
| feature_volume_populator: "true" |
🤖 Prompt for AI Agents
In operator/.kustomized_manifests around lines 6769-6772, the example CR uses
YAML booleans for feature flags; change each boolean value to the canonical
string "true" (e.g., feature_ui_plugin: "true", feature_validation: "true",
feature_volume_populator: "true") so the sample matches the CSV/init resource
convention and avoids advertising the boolean fallback.
Member
Author
There was a problem hiding this comment.
long term will be boolean
There was a problem hiding this comment.
@yaacov Got it—thanks. We’ll keep the example CR using booleans to match the long‑term direction. The CSV init resource here already uses booleans as well, so they’re consistent.
One follow-up: I previously recorded a learning that recommended strings as the canonical sample type. Your clarification supersedes that. Would you like me to remove the old “prefer canonical strings” learning and update guidance to:
- Canonical type: boolean for feature_* and related toggle fields
- Keep x-kubernetes-preserve-unknown-fields: true during the transition to accept string inputs for backward compatibility
- Samples/docs should show booleans
If you confirm, I’ll remove the obsolete learning(s).
✏️ Learnings added
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.393Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Member
Author
There was a problem hiding this comment.
yes please remove the learning
There was a problem hiding this comment.
Oops, something went wrong! Please try again later. 🐰 💔
Comment on lines
+8
to
+10
| feature_ui_plugin: true | ||
| feature_validation: true | ||
| feature_volume_populator: true |
There was a problem hiding this comment.
Revert sample values to canonical strings.
The CRD now tolerates booleans, but maintainer guidance is to keep docs/samples using the canonical string "true" so users stick with the long-term contract. Please revert these sample values to strings.
- feature_ui_plugin: true
- feature_validation: true
- feature_volume_populator: true
+ feature_ui_plugin: "true"
+ feature_validation: "true"
+ feature_volume_populator: "true"Based on learnings
📝 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.
Suggested change
| feature_ui_plugin: true | |
| feature_validation: true | |
| feature_volume_populator: true | |
| feature_ui_plugin: "true" | |
| feature_validation: "true" | |
| feature_volume_populator: "true" |
🤖 Prompt for AI Agents
In operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml around lines
8 to 10 the sample fields feature_ui_plugin, feature_validation, and
feature_volume_populator are set as bare booleans; revert them to the canonical
string form by changing true to "true" for each of these keys so the sample uses
the documented string contract.
Member
Author
There was a problem hiding this comment.
long term will be boolean
There was a problem hiding this comment.
@yaacov, understood! Thank you for clarifying the long-term direction. I'll note that the boolean values in the sample are aligned with the future state of the API.
✏️ Learnings added
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.582Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:154-182
Timestamp: 2025-10-16T13:20:28.803Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD boolean-like fields (e.g., feature_* and controller_* toggles) via x-kubernetes-preserve-unknown-fields to accept both strings and booleans, maintainers prefer NOT to document both types per field. Guidance should steer users to the canonical string "true"/"false", with boolean acceptance treated as temporary/backward-compatibility and likely to be removed in the next version.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
6422cbd to
b16861a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
operator/.upstream_manifests (1)
85-85: Numeric counters via x-kubernetes-int-or-string — LGTMAccepts ints and numeric strings as intended for these fields; matches the repository’s preferred approach.
Based on learnings
Also applies to: 88-88, 107-107, 110-110, 117-117, 120-120, 123-123, 130-130, 137-137, 140-140, 143-143, 150-150, 153-153, 341-341
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
operator/.downstream_manifests(7 hunks)operator/.kustomized_manifests(7 hunks)operator/.upstream_manifests(7 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(4 hunks)operator/config/manifests/bases/forklift-operator.clusterserviceversion.yaml(1 hunks)operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml(1 hunks)operator/streams/downstream/mtv-operator.clusterserviceversion.yaml(1 hunks)operator/streams/upstream/forklift-operator.clusterserviceversion.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.393Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.582Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
📚 Learning: 2025-10-16T13:45:36.153Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-10-16T15:58:40.393Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.393Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-10-16T13:44:44.382Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-10-16T15:57:23.582Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.582Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-10-16T13:46:22.250Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-09-18T13:48:36.037Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-09-01T16:32:31.284Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:151-153
Timestamp: 2025-09-01T16:32:31.284Z
Learning: The field controller_snapshot_removal_timeout_minuts in ForkliftController CRD contains a typo - it should be controller_snapshot_removal_timeout_minutes. The maintainer yaacov prefers to handle such fixes in separate issues rather than mixing them with documentation PRs.
Applied to files:
operator/.downstream_manifests
📚 Learning: 2025-10-16T13:44:49.699Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-10-16T14:11:57.015Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.upstream_manifests
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-03T07:08:53.741Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
Applied to files:
operator/.downstream_manifestsoperator/.kustomized_manifests
📚 Learning: 2025-09-01T16:36:42.418Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T14:02:10.056Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
Applied to files:
operator/.downstream_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-18T19:05:05.746Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Applied to files:
operator/.downstream_manifestsoperator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/config/samples/forklift_v1beta1_forkliftcontroller.yamloperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-10-16T01:41:12.346Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.kustomized_manifests:304-323
Timestamp: 2025-10-16T01:41:12.346Z
Learning: In Forklift operator CRD/manifests (e.g., operator/.kustomized_manifests and related bases/downstream files), maintainers prefer field descriptions that add explanatory detail rather than repeating the field name verbatim (e.g., for ova_proxy_fqin, describe the proxy’s purpose and behavior).
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-09-03T04:08:05.745Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.downstream_manifests:2523-2534
Timestamp: 2025-09-03T04:08:05.745Z
Learning: For Forklift Plan CRD docs, maintainer prefers documenting existing runtime keys/behavior verbatim (e.g., TargetLabels uses "app", convertor pods use "forklift.app") and avoiding normalization changes in documentation-only PRs.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-08-18T13:45:08.364Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:694-699
Timestamp: 2025-08-18T13:45:08.364Z
Learning: The user yaacov prefers not to change existing behavior within the scope of a current PR when additional validation or safety checks are identified, and instead prefers to create follow-up issues to address such improvements separately.
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:34:51.909Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:233-236
Timestamp: 2025-09-01T16:34:51.909Z
Learning: The maintainer yaacov prefers to keep PRs focused on their intended scope and handle different types of changes (like typo fixes vs documentation updates) in separate issues/PRs. When documenting existing APIs, the documentation should accurately reflect the current implementation state, even if it contains typos that will be fixed later.
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / forklift-operator-bundle-dev-preview-on-pull-request
- GitHub Check: build
🔇 Additional comments (13)
operator/.downstream_manifests (4)
6770-6772: Samples use boolean valuesKeeping example CR booleans aligns with the long‑term canonical type.
Based on learnings
6913-6916: CSV initialization resource uses booleansConsistent with schema and desired UI behavior.
Based on learnings
85-85: Approve schema relaxations for numeric and boolean fields
- Numeric counters switched to
x-kubernetes-int-or-string: true(back-compat accepts both 10 and “10”).- Feature toggles use
type: boolean+x-kubernetes-preserve-unknown-fields: true(accepts true/“true”).
Confirm CRD remains structural and scorecard passes without warnings.
126-134: Schema audit confirms all 15 flags correctly configuredVerification complete: All named flags have both
type: booleanandx-kubernetes-preserve-unknown-fields: trueapplied consistently. The schema pattern aligns with the agreed approach (per learnings) and supports string input tolerance alongside boolean types.The request to verify apiserver acceptance on target clusters is appropriate runtime validation and not a code issue.
operator/.kustomized_manifests (3)
85-85: Good move to x-kubernetes-int-or-string.This matches K8s structural schema and avoids invalid unions. LGTM.
Based on learnings
Also applies to: 88-88, 107-107, 110-110, 117-117, 120-120, 123-123, 130-130, 137-137, 140-140, 143-143, 150-150, 153-153, 341-341
126-127: Boolean toggles pattern acknowledged; please confirm string fallback works.The type: boolean + x-kubernetes-preserve-unknown-fields: true approach aligns with the agreed temporary relaxation and fixes UI rendering. Please double‑check on supported OCP/K8s that string values "true"/"false" are accepted for these fields.
Based on learnings
Also applies to: 133-134, 146-147, 156-157, 160-161, 164-165, 168-169, 172-173, 177-178, 181-182, 185-186, 189-190, 193-194, 237-238, 364-365
6770-6772: Samples/CSV use booleans — aligned with long‑term direction.Consistent with the canonical boolean type decision. LGTM.
Based on learnings
Also applies to: 6914-6916
operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml (1)
8-10: Samples aligned with canonical boolean direction.Using booleans here matches the stated long‑term type for feature flags.
Based on learnings
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (1)
249-283: LGTM: x-kubernetes-int-or-string for counters/intervals/levels.This correctly enables numeric literals and numeric strings for these fields.
Based on learnings
Also applies to: 303-309, 393-396
operator/streams/downstream/mtv-operator.clusterserviceversion.yaml (1)
37-39: Initialization resource matches boolean feature flags.Consistent with CRD shift to booleans; good.
operator/streams/upstream/forklift-operator.clusterserviceversion.yaml (1)
37-39: Initialization resource matches boolean feature flags.Consistent with CRD shift to booleans; good.
operator/.upstream_manifests (1)
126-127: Boolean flags with preserveUnknownFields — LGTMUsing type: boolean with x-kubernetes-preserve-unknown-fields: true on feature_* and controller_* toggles matches the agreed temporary relaxation and fixes the console UI rendering while tolerating legacy string inputs. No further runtime changes needed.
Based on learnings
Also applies to: 133-134, 146-147, 156-157, 160-161, 164-165, 168-169, 172-173, 177-178, 181-182, 185-186, 189-190, 193-194, 237-238, 364-365
operator/config/manifests/bases/forklift-operator.clusterserviceversion.yaml (1)
33-35: Verify boolean flag handling and CSV/CRD consistency
Aligns with canonical boolean use. Confirm:
- operator templates/roles do not string‐compare these flags
- CSVs use JSON booleans consistently
- CRD schema tolerates booleans (with x-kubernetes-preserve-unknown-fields for back-compat)
Comment on lines
+6770
to
+6772
| feature_ui_plugin: true | ||
| feature_validation: true | ||
| feature_volume_populator: true |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Samples/CSV init switched to booleans — looks good (optional consistency note)
Good to show canonical booleans. Optionally, consider aligning exposure of feature_* in samples/CSV (bundle all or keep minimal) in a dedicated follow-up, per repo preference.
Based on learnings
Also applies to: 6913-6916
🤖 Prompt for AI Agents
In operator/.upstream_manifests around lines 6770-6772 (and also update lines
6913-6916), the samples/CSV entries list feature_* fields that were switched to
canonical booleans; make these entries consistent by ensuring feature_ui_plugin,
feature_validation, and feature_volume_populator are set as true/false booleans
(not strings) in both locations, and if you prefer a consistent exposure
strategy, create a follow-up change that either includes all feature_* flags in
samples/CSV or reduces them to a minimal set across the file and document that
decision in the PR description.
Member
Author
There was a problem hiding this comment.
I do not want to change the example in this PR
There was a problem hiding this comment.
@yaacov Acknowledged. We’ll keep the example unchanged in this PR and skip any additional alignment here. Not opening a follow-up issue (noted your preference to track elsewhere). I’ve updated my knowledge base accordingly.
✏️ Learnings added
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:6770-6772
Timestamp: 2025-10-16T18:19:40.835Z
Learning: Repo: kubev2v/forklift — For PRs relaxing ForkliftController CRD types (e.g., accepting booleans/ints/strings), do not modify or expand the example ForkliftController resource in operator/.upstream_manifests within that PR. Keep examples as-is and handle any exposure/consistency adjustments (e.g., listing feature_* flags in samples/CSV) in a separate, dedicated follow-up. Avoid creating GitHub issues for such follow-ups as they’re tracked elsewhere.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.393Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.582Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Comment on lines
46
to
57
| feature_ui_plugin: | ||
| type: string | ||
| enum: ["true", "false"] | ||
| type: boolean | ||
| x-kubernetes-preserve-unknown-fields: true | ||
| description: "Enable UI plugin (default: true)" | ||
| feature_validation: | ||
| type: string | ||
| enum: ["true", "false"] | ||
| type: boolean | ||
| x-kubernetes-preserve-unknown-fields: true | ||
| description: "Enable validation service (default: true)" | ||
| feature_volume_populator: | ||
| type: string | ||
| enum: ["true", "false"] | ||
| type: boolean | ||
| x-kubernetes-preserve-unknown-fields: true | ||
| description: "Enable volume populators (default: true)" |
There was a problem hiding this comment.
🧩 Analysis chain
Boolean + preserve-unknown-fields will not accept string "true"/"false" → breaks back‑compat.
Kubernetes enforces the declared type. With type: boolean, string values are rejected. x-kubernetes-preserve-unknown-fields does not create a boolean|string union for scalars. If the goal is to accept both booleans and strings temporarily, this schema won’t achieve it.
Options:
- Keep back‑compat: drop type for these fields and keep only x-kubernetes-preserve-unknown-fields: true (accepts any scalar but loses console boolean toggle), or
- Commit to booleans now (reject strings) and update docs accordingly.
Please confirm the intended contract and adjust accordingly.
Based on learnings
Also applies to: 59-69, 71-81, 286-300, 328-330, 402-404
🌐 Web query:
In Kubernetes CRDs, does setting type: boolean together with x-kubernetes-preserve-unknown-fields: true allow string values (e.g., "true") to be accepted for that property?
💡 Result:
Short answer: No. If you declare type: boolean the API server expects a JSON boolean (true/false); setting x-kubernetes-preserve-unknown-fields: true prevents pruning of unknown object fields but does not make a boolean-typed property accept the string "true". [1][2]
Sources:
- Kubernetes structural-schema / preserve-unknown-fields explanation. [1]
- CRD validation / types (int-or-string is the special case that allows strings for numbers, not preserve-unknown-fields). [2]
The schema will not accept string values with type: boolean—this is a critical issue.
The web search confirms your concern: Kubernetes enforces declared types. Setting type: boolean means the API server rejects non-boolean values (including "true" as a string). The x-kubernetes-preserve-unknown-fields: true attribute only prevents pruning of unknown object fields; it does not create a type union for scalars.
Per the learnings, the canonical type for feature flags is boolean. To support backward compatibility with string inputs, remove the type field entirely and keep only x-kubernetes-preserve-unknown-fields: true—this will accept any scalar, but you lose the Kubernetes console boolean toggle. Alternatively, commit to booleans only and update all user-facing docs/examples to use canonical boolean syntax.
Choose one approach and update all affected fields (lines 46–57, 59–69, 71–81, 286–300, 328–330, 402–404).
🤖 Prompt for AI Agents
In operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
around lines 46–57 (and also update the other affected ranges 59–69, 71–81,
286–300, 328–330, 402–404), the fields currently declare "type: boolean" which
will reject string values like "true"; to preserve backward compatibility remove
the "type: boolean" lines for these feature_* entries and leave only
"x-kubernetes-preserve-unknown-fields: true" and the description so the CRD will
accept scalar strings, applying this same edit to every listed range; ensure no
other scalar type declarations remain for these feature flags.
Comment on lines
34
to
35
| "namespace": ${NAMESPACE} | ||
| }, |
There was a problem hiding this comment.
Fix invalid JSON: quote ${NAMESPACE}.
In operatorframework.io/initialization-resource JSON, namespace must be a JSON string:
- "namespace": ${NAMESPACE}
+ "namespace": "${NAMESPACE}"📝 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.
Suggested change
| "namespace": ${NAMESPACE} | |
| }, | |
| "namespace": "${NAMESPACE}" | |
| }, |
🤖 Prompt for AI Agents
In operator/streams/upstream/forklift-operator.clusterserviceversion.yaml around
lines 34 to 35, the JSON for operatorframework.io/initialization-resource uses
${NAMESPACE} unquoted which produces invalid JSON; change it to a JSON string by
surrounding the variable with double quotes (i.e. "namespace": "${NAMESPACE}"),
ensuring the resulting YAML/JSON renders a valid string value for namespace.
Member
Author
There was a problem hiding this comment.
thank you this is a nice catch, we will fix not in this PR, can you open an issue for that ?
There was a problem hiding this comment.
🧩 Analysis chain
@yaacov, I'll open a GitHub issue for this. Let me create it now.
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo kubev2v/forklift \
--title "Invalid JSON in operatorframework.io/initialization-resource: unquoted namespace variable" \
--body "## Description
In the ClusterServiceVersion files, the \`operatorframework.io/initialization-resource\` annotation contains JSON where the \`namespace\` field uses an unquoted variable placeholder \`\${NAMESPACE}\`, which produces invalid JSON when not properly quoted.
## Files Affected
- \`operator/streams/upstream/forklift-operator.clusterserviceversion.yaml\`
- \`operator/streams/downstream/mtv-operator.clusterserviceversion.yaml\` (likely)
- \`operator/config/manifests/bases/forklift-operator.clusterserviceversion.yaml\` (likely)
## Issue
The namespace field should be a proper JSON string with the variable quoted.
## Context
Identified during review of PR #3204.
Comment: https://github.com/kubev2v/forklift/pull/3204#discussion_r2436629164
Reported by: @yaacov" \
--assignee yaacov
Length of output: 47
@yaacov, I've created issue #3209 to track fixing the unquoted ${NAMESPACE} variable in the JSON annotation. The issue has been assigned to you and includes links back to this PR and comment for context.
🐰
🧠 Learnings used
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:6770-6772
Timestamp: 2025-10-16T18:19:40.835Z
Learning: Repo: kubev2v/forklift — For PRs relaxing ForkliftController CRD types (e.g., accepting booleans/ints/strings), do not modify or expand the example ForkliftController resource in operator/.upstream_manifests within that PR. Keep examples as-is and handle any exposure/consistency adjustments (e.g., listing feature_* flags in samples/CSV) in a separate, dedicated follow-up. Avoid creating GitHub issues for such follow-ups as they’re tracked elsewhere.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.699Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.393Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.250Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.582Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.015Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.153Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.382Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Member
Author
|
converting to draft, after discussion with @mnecas we decided to use strict a. use strict API (string), while allowing easy transition from 2.9 where we used strings "true" / "false" , but also allowed booleans. |
yaacov
force-pushed
the
forklift-controller-crd-allow-unknown-types-for-feature-flags
branch
4 times, most recently
from
f8717af to
3d271a4
Compare
yaacov
force-pushed
the
forklift-controller-crd-allow-unknown-types-for-feature-flags
branch
6 times, most recently
from
ee067f3 to
e329241
Compare
Hide forklift controller spec fields in UI automatic form Signed-off-by: yaacov <kobi.zamir@gmail.com>
yaacov
force-pushed
the
forklift-controller-crd-allow-unknown-types-for-feature-flags
branch
from
e329241 to
9c77302
Compare
|
Member
Author
|
Update:
screenshots: 
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
operator/.downstream_manifests (1)
142-144: Known typo retained by designcontroller_snapshot_removal_timeout_minuts: acknowledged typo; no change requested in this PR.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
hack/validate_forklift_controller_crd.py(1 hunks)operator/.downstream_manifests(10 hunks)operator/.kustomized_manifests(10 hunks)operator/.upstream_manifests(10 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(5 hunks)operator/config/manifests/bases/forklift-operator.clusterserviceversion.yaml(1 hunks)operator/streams/downstream/mtv-operator.clusterserviceversion.yaml(1 hunks)operator/streams/upstream/forklift-operator.clusterserviceversion.yaml(1 hunks)
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.425Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:6770-6772
Timestamp: 2025-10-16T18:19:40.853Z
Learning: Repo: kubev2v/forklift — For PRs relaxing ForkliftController CRD types (e.g., accepting booleans/ints/strings), do not modify or expand the example ForkliftController resource in operator/.upstream_manifests within that PR. Keep examples as-is and handle any exposure/consistency adjustments (e.g., listing feature_* flags in samples/CSV) in a separate, dedicated follow-up. Avoid creating GitHub issues for such follow-ups as they’re tracked elsewhere.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.112Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.727Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.273Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.177Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.405Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.606Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:40-40
Timestamp: 2025-09-01T14:02:10.056Z
Learning: The ForkliftController CRD intentionally uses `additionalProperties: true` in its OpenAPI schema to maintain flexibility for future field additions without requiring schema updates. This is a deliberate architectural decision to support extensibility.
📚 Learning: 2025-09-18T19:05:05.746Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:187-204
Timestamp: 2025-09-18T19:05:05.746Z
Learning: ForkliftController CRD validation philosophy (kubev2v/forklift): The CRD schema is designed for hinting/documenting available configuration knobs rather than strict validation. String fields accept any values, and actual validation occurs where values are used (in Ansible roles/templates). The hack/validate_forklift_controller_crd.py focuses on defaults-CRD consistency, not enforcing strict field validation.
Applied to files:
hack/validate_forklift_controller_crd.pyoperator/.kustomized_manifestsoperator/.downstream_manifests
📚 Learning: 2025-10-16T18:19:40.853Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:6770-6772
Timestamp: 2025-10-16T18:19:40.853Z
Learning: Repo: kubev2v/forklift — For PRs relaxing ForkliftController CRD types (e.g., accepting booleans/ints/strings), do not modify or expand the example ForkliftController resource in operator/.upstream_manifests within that PR. Keep examples as-is and handle any exposure/consistency adjustments (e.g., listing feature_* flags in samples/CSV) in a separate, dedicated follow-up. Avoid creating GitHub issues for such follow-ups as they’re tracked elsewhere.
Applied to files:
hack/validate_forklift_controller_crd.pyoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/streams/downstream/mtv-operator.clusterserviceversion.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:44:49.727Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:138-138
Timestamp: 2025-10-16T13:44:49.727Z
Learning: Repo: kubev2v/forklift — For PRs that relax ForkliftController CRD types (e.g., boolean-like and counting fields), maintainers prefer no field-name changes. Typos like controller_snapshot_removal_timeout_minuts should be tracked in a separate follow-up issue rather than addressed in the same PR.
Applied to files:
hack/validate_forklift_controller_crd.pyoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:46:22.273Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T13:46:22.273Z
Learning: Repo: kubev2v/forklift — When temporarily relaxing ForkliftController CRD types to accept booleans/ints alongside strings, maintainers prefer not to add runtime normalization or extra checks in roles/templates. Keep changes limited to schema, treat this as temporary compatibility, and plan to re-tighten types in a follow-up release.
Applied to files:
hack/validate_forklift_controller_crd.pyoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:36:42.418Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:384-384
Timestamp: 2025-09-01T16:36:42.418Z
Learning: The maintainer yaacov has confirmed that additionalProperties: true in the ForkliftController CRD spec is intentionally kept to allow new features to be added to the controller without requiring CRD schema updates. This is a deliberate architectural choice for extensibility.
Applied to files:
hack/validate_forklift_controller_crd.pyoperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T14:11:57.112Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.upstream_manifests:156-181
Timestamp: 2025-10-16T14:11:57.112Z
Learning: Repo: kubev2v/forklift — Maintainer yaacov prefers not to create GitHub follow-up issues (e.g., for re-tightening ForkliftController CRD types after temporary relaxation); such work is tracked in a different system. In future reviews, acknowledge and proceed without proposing GitHub issue creation.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T15:58:40.425Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:6769-6772
Timestamp: 2025-10-16T15:58:40.425Z
Learning: Repo: kubev2v/forklift — Maintainer clarified that the long-term canonical type for ForkliftController feature flags (feature_*, and related controller_* toggles) is boolean. Samples should show booleans, while the CRD may keep x-kubernetes-preserve-unknown-fields: true temporarily to accept string inputs for backward compatibility.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T13:45:36.177Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.downstream_manifests:126-126
Timestamp: 2025-10-16T13:45:36.177Z
Learning: Repo: kubev2v/forklift — For CRD schema in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (and kustomized/downstream manifests), do not propose OpenAPI unions like anyOf/oneOf to accept multiple scalar types; they are not accepted by Kubernetes CRD structural schema. To temporarily allow both strings and booleans on feature/toggle fields, use x-kubernetes-preserve-unknown-fields: true, and for numeric counters use x-kubernetes-int-or-string: true. Keep user-facing docs/examples using canonical string "true"/"false".
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-16T15:57:23.606Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/config/samples/forklift_v1beta1_forkliftcontroller.yaml:8-10
Timestamp: 2025-10-16T15:57:23.606Z
Learning: Repo: kubev2v/forklift — For ForkliftController CRD feature flags and boolean-like fields, the long-term plan is to use boolean types (true/false) rather than strings. Samples and documentation should use boolean values to align with this direction, not string "true"/"false".
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.kustomized_manifestsoperator/.downstream_manifests
📚 Learning: 2025-10-16T13:44:44.405Z
Learnt from: yaacov
PR: kubev2v/forklift#3204
File: operator/.kustomized_manifests:124-153
Timestamp: 2025-10-16T13:44:44.405Z
Learning: Repo: kubev2v/forklift — For CRD unions, do not suggest OpenAPI oneOf/anyOf; Kubernetes structural schemas reject these for type unions. Use x-kubernetes-int-or-string for numeric unions and, for boolean-like ForkliftController flags (feature_* and controller_* plus k8s_cluster and virt_v2v_dont_request_kvm), rely on x-kubernetes-preserve-unknown-fields to tolerate both booleans and strings while documentation continues to prefer "true"/"false".
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-03T07:08:53.741Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.kustomized_manifests:2522-2524
Timestamp: 2025-09-03T07:08:53.741Z
Learning: Forklift Plan CRD: Both Plan.spec.targetAffinity and Plan.spec.convertorAffinity are intentionally annotated as atomic (+structType=atomic), resulting in x-kubernetes-map-type: atomic in the CRD YAML (operator/.kustomized_manifests and operator/config/crd/bases). Do not flag these as incorrect in future reviews.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/.kustomized_manifestsoperator/.downstream_manifests
📚 Learning: 2025-10-16T01:41:12.362Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.kustomized_manifests:304-323
Timestamp: 2025-10-16T01:41:12.362Z
Learning: In Forklift operator CRD/manifests (e.g., operator/.kustomized_manifests and related bases/downstream files), maintainers prefer field descriptions that add explanatory detail rather than repeating the field name verbatim (e.g., for ova_proxy_fqin, describe the proxy’s purpose and behavior).
Applied to files:
operator/config/manifests/bases/forklift-operator.clusterserviceversion.yamloperator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/streams/upstream/forklift-operator.clusterserviceversion.yamloperator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-10-15T22:30:05.320Z
Learnt from: mansam
PR: kubev2v/forklift#3183
File: operator/.downstream_manifests:311-317
Timestamp: 2025-10-15T22:30:05.320Z
Learning: In operator manifests (e.g., operator/.downstream_manifests) for ForkliftController CRD fields, maintainers prefer that example values do not need to match the documented defaults; examples can illustrate non-default configurations (e.g., ova_proxy_container_requests_cpu and ova_proxy_container_requests_memory examples differ from their defaults).
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-09-18T13:48:36.037Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.037Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Applied to files:
operator/.kustomized_manifestsoperator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-08-18T13:45:08.364Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:694-699
Timestamp: 2025-08-18T13:45:08.364Z
Learning: The user yaacov prefers not to change existing behavior within the scope of a current PR when additional validation or safety checks are identified, and instead prefers to create follow-up issues to address such improvements separately.
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:34:51.909Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:233-236
Timestamp: 2025-09-01T16:34:51.909Z
Learning: The maintainer yaacov prefers to keep PRs focused on their intended scope and handle different types of changes (like typo fixes vs documentation updates) in separate issues/PRs. When documenting existing APIs, the documentation should accurately reflect the current implementation state, even if it contains typos that will be fixed later.
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-01T16:32:31.284Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:151-153
Timestamp: 2025-09-01T16:32:31.284Z
Learning: The field controller_snapshot_removal_timeout_minuts in ForkliftController CRD contains a typo - it should be controller_snapshot_removal_timeout_minutes. The maintainer yaacov prefers to handle such fixes in separate issues rather than mixing them with documentation PRs.
Applied to files:
operator/.downstream_manifests
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / forklift-operator-bundle-dev-preview-on-pull-request
- GitHub Check: lint
- GitHub Check: build
🔇 Additional comments (16)
hack/validate_forklift_controller_crd.py (1)
195-213: LGTM: Validation script correctly handles new CRD-only properties.The
allowed_crd_only_propertiesset appropriately excludes the new configuration fields (inventory_route_timeout, metric_interval, ova_proxy_route_timeout, and ConfigMap names) from task-usage validation. These fields are consumed indirectly through templates and don't appear in direct task variable references, so this exclusion is correct.The implementation cleanly separates the two validation directions: CRD properties not in tasks are filtered through the allowlist, while task variables missing from the CRD still trigger validation failures.
operator/streams/upstream/forklift-operator.clusterserviceversion.yaml (1)
122-619: LGTM: Comprehensive specDescriptors for ForkliftController CRD.The specDescriptors block correctly exposes the expanded ForkliftController public surface:
- Feature gates (9 fields) use radio button descriptors for UI toggles
- Container images and advanced settings appropriately marked as hidden
- All new fields from this PR are included: inventory_route_timeout, metric_interval, ova_proxy_route_timeout, and the three ConfigMap name fields (ovirt_osmap, vsphere_osmap, virt_customize)
- Descriptor metadata aligns with the CRD schema changes throughout the PR
The UI integration will enable proper rendering of boolean feature flags while maintaining the schema flexibility for backward compatibility.
operator/streams/downstream/mtv-operator.clusterserviceversion.yaml (1)
118-615: LGTM: Downstream specDescriptors consistent with upstream.The specDescriptors block correctly mirrors the upstream version, maintaining consistency across distributions. All new fields (inventory_route_timeout, metric_interval, ova_proxy_route_timeout, ConfigMap names) are properly included with appropriate UI metadata.
Note: Line 34 correctly quotes the namespace variable as
"${NAMESPACE}", which produces valid JSON (unlike the upstream version at line 34 which will be fixed separately per issue #3209).operator/config/manifests/bases/forklift-operator.clusterserviceversion.yaml (1)
52-549: LGTM: Base manifest specDescriptors properly structured.The specDescriptors in the base manifest correctly define the ForkliftController CRD public surface. All new fields from this PR are present:
- inventory_route_timeout (221-225)
- metric_interval (545-549)
- ova_proxy_route_timeout (523-527)
- ConfigMap names: ovirt_osmap_configmap_name, vsphere_osmap_configmap_name, virt_customize_configmap_name (326-340)
This base manifest serves as the source for the upstream/downstream generated versions, and the structure is consistent across all files.
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (3)
46-81: LGTM: Feature gates maintain strict string types as decided.The feature gate fields correctly use
type: stringwithenum: ["true", "false"]constraints. This aligns with the PR objectives where maintainers decided to use strict string types for feature gates rather than relaxing to booleans, enabling migration from previous releases while maintaining a clear API contract.Based on learnings
176-179: LGTM: New configuration fields properly added.The new fields are correctly defined with appropriate types:
inventory_route_timeout(176-179): string type for duration valuesvalidation_extra_volume_nameandvalidation_extra_volume_mountpath(255-262): string types for Kubernetes resource names and paths- ConfigMap name fields (264-276):
ovirt_osmap_configmap_name,vsphere_osmap_configmap_name,virt_customize_configmap_nameas string typesova_proxy_route_timeout(418-421): string type for duration valuesmetric_interval(436-440): string type for interval durationAll fields include clear descriptions, examples, and defaults. These align with the
allowed_crd_only_propertiesset in the validation script.Also applies to: 255-276, 418-421, 436-440
253-253: LGTM: Numeric fields correctly converted to int-or-string.All numeric counters, intervals, timeouts, and percentages now use
x-kubernetes-int-or-string: true:
- validation_policy_agent_search_interval (253)
- Migration settings: controller_max_vm_inflight, controller_precopy_interval, controller_max_concurrent_reconciles, controller_snapshot_removal_timeout_minuts, controller_snapshot_status_check_rate_seconds, controller_cleanup_retries, controller_dv_status_check_retries, controller_snapshot_removal_check_retries, controller_vddk_job_active_deadline_sec, controller_tls_connection_timeout_sec (280-308)
- Storage overhead: controller_filesystem_overhead, controller_block_overhead (330-334)
- controller_log_level (425)
This enables accepting both integer values and numeric strings, providing flexibility for users while the existing Ansible roles/templates handle type conversion. As confirmed in past reviews, this approach avoids OpenAPI unions and properly uses Kubernetes' built-in int-or-string capability.
Based on learnings
Also applies to: 280-308, 330-334, 425-425
operator/.kustomized_manifests (4)
83-123: LGTM: Adopted x-kubernetes-int-or-string on numeric knobs.Consistent application on controller_* and validation_* counters/times looks correct and aligns with structural-schema constraints (no oneOf/anyOf). No issues found.
Also applies to: 130-133, 139-147, 154-159, 391-394
323-326: LGTM: ConfigMap name fields added.Names and defaults are clear; no schema concerns.
Also applies to: 398-402, 436-439
6827-6836: LGTM: Example and init resource use string "true" for feature_ flags.*Matches the strict string enum type chosen for this release.
Also applies to: 6969-6981
6997-7476: CSV specDescriptors: good coverage; verify console writes string "true"/"false".Descriptors look consistent and map to existing CRD paths. Please sanity-check in the Console that radio:true/false persists string values (not booleans) for these string-typed flags.
operator/.downstream_manifests (3)
260-264: New config/time fields look consistentString-typed with examples and descriptions; aligns with existing schema style.
Also applies to: 271-275, 319-323, 324-327, 379-386, 398-402, 436-439
85-85: Counters: switching to x-kubernetes-int-or-string is goodVerification confirms all counter/timeout fields now correctly include
x-kubernetes-int-or-string: trueannotation. This aligns with Kubernetes CRD conventions and maintainer guidance for accepting numeric-like inputs. No further changes requested.
6996-7475: CSV specDescriptor coverage requires verificationThe specDescriptor paths shown (feature flags, images, resource limits, timeouts) appear well-structured with appropriate radio and hidden descriptors. However, the verification script failed to confirm all paths actually exist in the ForkliftController CRD properties. Manually verify that each of the ~100+ path references (e.g.,
feature_ui_plugin,controller_image_fqin,controller_max_vm_inflight, etc.) are present in the corresponding CRD spec.operator/.upstream_manifests (2)
85-85: Int-or-String is the right call; add explicittype: stringfor structural compliance.Great move switching numeric counters to
x-kubernetes-int-or-string: true. To keep the CRD strictly structural and avoid any schema validator edge cases, addtype: stringon each of these fields alongside the extension.Example (apply similarly to all listed lines):
controller_block_overhead: description: 'Block overhead in bytes (default: 0)' + type: string x-kubernetes-int-or-string: trueFields affected in this file: lines 85, 88, 107, 110, 117, 120, 123, 132, 141, 144, 147, 156, 159, and 393.
Please confirm scorecard “olm-crds-have-validation” passes with/without this addition, and that you prefer adding
type: stringover relying on the extension alone. Based on learningsAlso applies to: 88-88, 107-107, 110-110, 117-117, 120-120, 123-123, 132-132, 141-141, 144-144, 147-147, 156-156, 159-159, 393-393
6996-7475: CSV specDescriptors for feature toggles and controller fields — aligned and clean.
- Descriptors map correctly to string-enum fields; no
anyOf/oneOfused.- Keeping
controller_snapshot_removal_timeout_minutsas-is is fine for back-compat (typo acknowledged elsewhere).Two quick checks:
- Ensure the OpenShift/OLM console “radio:true/false” renders quoted strings (“true”/“false”) for these string-typed fields, not booleans.
- Run scorecard “olm-spec-descriptors” to validate descriptor coverage.
If helpful, I can provide a quick grep script to sanity-check that each
path:here has a corresponding property in the CRD marked astype: string. Based on learnings
Comment on lines
+260
to
264
| inventory_route_timeout: | ||
| description: 'Inventory route timeout (default: 360s)' | ||
| example: 600s | ||
| type: string | ||
| k8s_cluster: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify accepted duration format (seconds with “s” suffix).
These string timeouts/intervals use examples like 360s/600s. Tighten descriptions to state the expected format to reduce user confusion.
- description: 'Inventory route timeout (default: 360s)'
+ description: 'Inventory route timeout in seconds as a duration string with "s" suffix (default: 360s, e.g., "600s")'
@@
- description: 'Metrics scrape interval (default: 30s)'
+ description: 'Metrics scrape interval in seconds as a duration string with "s" suffix (default: 30s, e.g., "60s")'
@@
- description: 'OVA Proxy route timeout (default: 360s)'
+ description: 'OVA Proxy route timeout in seconds as a duration string with "s" suffix (default: 360s, e.g., "600s")'Also applies to: 271-274, 319-322
🤖 Prompt for AI Agents
operator/.kustomized_manifests around lines 260-264 (also apply same change to
271-274 and 319-322): the timeout/interval description is ambiguous about
accepted duration format; update the description strings to explicitly state the
expected format is a duration in seconds with an "s" suffix (for example "360s"
or "600s") and ensure the example values match that format (e.g., example:
360s). Make the wording concise (e.g., "Inventory route timeout — duration in
seconds with 's' suffix (default: 360s)") and apply the same pattern to the
other referenced blocks.
Comment on lines
+379
to
+386
| validation_extra_volume_mountpath: | ||
| description: 'Mount path for extra validation rules (default: /usr/share/opa/policies/extra)' | ||
| example: /custom/validation/path | ||
| type: string | ||
| validation_extra_volume_name: | ||
| description: 'Volume name for extra validation rules (default: validation-extra-rules)' | ||
| example: custom-validation-rules | ||
| type: string |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make mount path constraints explicit.
Minor doc polish to set user expectations that this path is inside the validation container and should be absolute.
- description: 'Mount path for extra validation rules (default: /usr/share/opa/policies/extra)'
+ description: 'Absolute mount path inside the validation container for extra rules (default: /usr/share/opa/policies/extra)'
Comment on lines
+260
to
+263
| inventory_route_timeout: | ||
| description: 'Inventory route timeout (default: 360s)' | ||
| example: 600s | ||
| type: string |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Clarify units in descriptions for new timeout/config fields.
To reduce ambiguity in UIs and docs, explicitly call out units (“seconds”) and accepted form in these descriptions.
Example tweak:
- description: 'Inventory route timeout (default: 360s)'
+ description: 'Inventory route timeout in seconds (default: 360s). Use values like 600s.'Apply similarly to: inventory_route_timeout, metric_interval, ova_proxy_route_timeout, validation_extra_volume_mountpath/name (paths are fine), virt_customize_configmap_name, ovirt_osmap_configmap_name, vsphere_osmap_configmap_name.
Based on learnings
Also applies to: 271-274, 319-322, 323-326, 379-386, 398-402, 436-439
🤖 Prompt for AI Agents
In operator/.upstream_manifests around lines 260-263 (and similarly for ranges
271-274, 319-322, 323-326, 379-386, 398-402, 436-439), the field descriptions
for timeout/interval/config fields are ambiguous about units; update each
description to explicitly state the expected unit ("seconds") and accepted
format (e.g., numeric with suffix "s" or plain seconds) — for example change
"Inventory route timeout (default: 360s)" to "Inventory route timeout in seconds
(default: 360s). Accepts values with 's' suffix or plain seconds." Apply this
same pattern to inventory_route_timeout, metric_interval,
ova_proxy_route_timeout, validation_extra_volume_mountpath/name (note: leave
paths as-is), virt_customize_configmap_name, ovirt_osmap_configmap_name, and
vsphere_osmap_configmap_name so all timeout/interval fields clearly document
units and accepted form.
Member
Author
yaacov
changed the title
solenoci
approved these changes
Oct 22, 2025
Collaborator
solenoci
left a comment
solenoci
left a comment
There was a problem hiding this comment.
LGTM, let's not forget about the created issue.
Member
Author
|
/backport release-2.10 |
|
🔄 Backport Status Starting backport of PR #3204 to |
2 tasks
|
✅ Backport Completed Successfully PR #3204 has been successfully backported to A new pull request should have been created with the backported changes. |
yaacov
added a commit
that referenced
this pull request
Oct 22, 2025
…nstaller automated UI form. (#3271) **Backport:** #3204 Issue: After setting up an strict API, where feature flags are strings, the automated UI form got very big and included some fields we did not want to expose to the UI. Fix: Continue with relaxed type settings, - [x] Adding new fields to the forklift controller CRD - [x] Hide all fields except the feature flags from the UI form Notes: - YAML help include all fields - Since CRD and UI require deterministic types, we will only accept strings for boolean fields. Screenshots: Automated UI: <img width="1675" height="1172" alt="forkliftcontroller-form" src="https://github.com/user-attachments/assets/4e51a86d-ac3b-4d7c-bf83-960747fa7600" /> YAML form: <img width="1675" height="1172" alt="forkliftcontroller-yaml" src="https://github.com/user-attachments/assets/9d2670e9-1833-4d30-8c82-05fae5278fbb" /> Signed-off-by: yaacov <kobi.zamir@gmail.com> Co-authored-by: yaacov <kobi.zamir@gmail.com>
prantikng
pushed a commit
to prantikng/sopm
that referenced
this pull request
Dec 10, 2025
…ted UI form. (kubev2v#3204) Issue: After setting up an strict API, where feature flags are strings, the automated UI form got very big and included some fields we did not want to expose to the UI. Fix: Continue with relaxed type settings, - [x] Adding new fields to the forklift controller CRD - [x] Hide all fields except the feature flags from the UI form Notes: - YAML help include all fields - Since CRD and UI require deterministic types, we will only accept strings for boolean fields. Screenshots: Automated UI: <img width="1675" height="1172" alt="forkliftcontroller-form" src="https://github.com/user-attachments/assets/4e51a86d-ac3b-4d7c-bf83-960747fa7600" /> YAML form: <img width="1675" height="1172" alt="forkliftcontroller-yaml" src="https://github.com/user-attachments/assets/9d2670e9-1833-4d30-8c82-05fae5278fbb" /> Signed-off-by: yaacov <kobi.zamir@gmail.com>
This was referenced Apr 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue:
After setting up an strict API, where feature flags are strings, the automated UI form got very big and included some fields we did not want to expose to the UI.
Fix:
Continue with relaxed type settings,
Notes:
Screenshots:
Automated UI:
YAML form:
