OCPCLOUD-3318: Integrate CompatibilityRequirements into the CCAPIO Insaller (WIP)#604
OCPCLOUD-3318: Integrate CompatibilityRequirements into the CCAPIO Insaller (WIP)#604theobarberbany wants to merge 7 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
@theobarberbany: This pull request references OCPCLOUD-3318 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this: Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe PR threads ChangesUnmanaged CRD revision flow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/revisiongenerator/revision.go (2)
240-257:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix linter-flagged formatting issues.
Static analysis reports two issues:
- Missing blank line between the
vardeclaration (line 241) and theforloop (line 242)- Formatting issue at line 265
🔧 Proposed formatting fix
func (r *installerRevision) ToAPIRevision() (operatorv1alpha1.ClusterAPIInstallerRevision, error) { var apiComponents []operatorv1alpha1.ClusterAPIInstallerComponent + for _, component := range r.components { if component.synthetic { continue }Run
make fmtto fix all formatting issues.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/revisiongenerator/revision.go` around lines 240 - 257, In the ToAPIRevision method of the installerRevision type, add a blank line between the var apiComponents declaration and the for loop that iterates over r.components. Additionally, run make fmt to automatically fix all remaining formatting issues flagged by the linter, including the formatting issue at line 265.Source: Linters/SAST tools
402-410:⚠️ Potential issue | 🟡 MinorThe
syntheticfield filtering logic is never triggered because no components are created withsyntheticset totrue.The
renderedComponentstruct includes asyntheticfield that is checked inToAPIRevision()at line 243 to skip components from the API revision output. However, innewRenderedComponent()at lines 413-416, new components are instantiated without explicitly settingsynthetic, leaving it at the zero value (false). No code in the repository setssynthetictotrue, making the filtering condition dead code.The comment at line 317 ("objects in a synthetic component and filtered from their normal phases") suggests this filtering was intended for a feature that is not yet implemented. Either remove the unused field and filtering logic, or implement the feature to create synthetic components.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/revisiongenerator/revision.go` around lines 402 - 410, The `synthetic` field in the `renderedComponent` struct is never set to `true` anywhere in the codebase, making the filtering logic in `ToAPIRevision()` dead code. Either remove the unused `synthetic` field from the `renderedComponent` struct definition, delete the filtering check in `ToAPIRevision()` that skips components where synthetic is true, and remove the related comment at line 317, or implement the feature by identifying where synthetic components should actually be created and setting `synthetic` to `true` in `newRenderedComponent()` or elsewhere when instantiating those components.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controllers/installer/probes.go`:
- Around line 34-35: The probeSucceededPredicate logic stops at the first
matching probe when evaluating status transitions, which causes the second
compatibilityRequirementCompatibleProbe to be masked and never trigger reconcile
events. Both compatibilityRequirementAdmittedProbe and
compatibilityRequirementCompatibleProbe share the same GroupKind, so the
predicate stops after finding the first one. Modify the predicate logic to
evaluate both probes instead of stopping at the first match, ensuring that
status transitions for both the Admitted and Compatible conditions can properly
trigger reconciliation events.
---
Outside diff comments:
In `@pkg/revisiongenerator/revision.go`:
- Around line 240-257: In the ToAPIRevision method of the installerRevision
type, add a blank line between the var apiComponents declaration and the for
loop that iterates over r.components. Additionally, run make fmt to
automatically fix all remaining formatting issues flagged by the linter,
including the formatting issue at line 265.
- Around line 402-410: The `synthetic` field in the `renderedComponent` struct
is never set to `true` anywhere in the codebase, making the filtering logic in
`ToAPIRevision()` dead code. Either remove the unused `synthetic` field from the
`renderedComponent` struct definition, delete the filtering check in
`ToAPIRevision()` that skips components where synthetic is true, and remove the
related comment at line 317, or implement the feature by identifying where
synthetic components should actually be created and setting `synthetic` to
`true` in `newRenderedComponent()` or elsewhere when instantiating those
components.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5962f1c7-9a1a-41b9-b031-971b7f903227
📒 Files selected for processing (4)
pkg/controllers/installer/probes.gopkg/controllers/installer/revision_reconciler.gopkg/controllers/revision/revision_controller.gopkg/revisiongenerator/revision.go
theobarberbany
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/revisiongenerator/revision.go (1)
182-185: 💤 Low valueDefine a sentinel error for missing unmanaged CRDs.
The linter correctly flags that dynamic errors prevent programmatic error checking with
errors.Is(). Define a sentinel error to allow callers to distinguish this specific failure mode.♻️ Proposed fix
Add a package-level sentinel error:
var ErrUnmanagedCRDsNotFound = errors.New("unmanaged CRDs not found in any component")Then wrap it at line 184:
- return fmt.Errorf("unmanaged CRDs not found in any component: %v", sets.List(missing)) + return fmt.Errorf("%w: %v", ErrUnmanagedCRDsNotFound, sets.List(missing))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/revisiongenerator/revision.go` around lines 182 - 185, Define a package-level sentinel error ErrUnmanagedCRDsNotFound using errors.New() at the top of the revision.go file. Then modify the return statement where the error is currently returned to use fmt.Errorf with the %w verb to wrap the sentinel error while including the missing CRDs information, allowing callers to check for this specific error type using errors.Is().Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/revisiongenerator/compatibility.go`:
- Around line 49-50: The Name field construction at line 49 concatenates
compatibilityRequirementNamePrefix with crd.GetName() without validating the
total length, which can exceed Kubernetes' 253-character DNS-subdomain limit for
long CRD names. Add length validation to ensure the resulting name stays within
the limit. If the concatenated name would be too long, either truncate the CRD
name proportionally or use a hash-based suffix of the CRD name to keep the
overall name length valid and deterministic while preventing reconciliation
failures for long CRD names.
---
Nitpick comments:
In `@pkg/revisiongenerator/revision.go`:
- Around line 182-185: Define a package-level sentinel error
ErrUnmanagedCRDsNotFound using errors.New() at the top of the revision.go file.
Then modify the return statement where the error is currently returned to use
fmt.Errorf with the %w verb to wrap the sentinel error while including the
missing CRDs information, allowing callers to check for this specific error type
using errors.Is().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f8dec810-3119-4457-9b54-78dcd47dc009
📒 Files selected for processing (4)
pkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/revisiongenerator/compatibility.gopkg/revisiongenerator/revision.go
| Name: compatibilityRequirementNamePrefix + crd.GetName(), | ||
| }, |
There was a problem hiding this comment.
Guard against invalid CompatibilityRequirement names for long CRD names.
Line 49 can generate a metadata.name longer than Kubernetes’ max DNS-subdomain length when a CRD name is already near the limit. That will make the synthetic object invalid and break reconciliation for those unmanaged CRDs.
Suggested fix
+const maxK8sNameLen = 253
+
+func compatibilityRequirementName(crdName string) string {
+ name := compatibilityRequirementNamePrefix + crdName
+ if len(name) <= maxK8sNameLen {
+ return name
+ }
+
+ // keep deterministic name while preserving uniqueness
+ sum := sha256.Sum256([]byte(crdName))
+ suffix := "-" + hex.EncodeToString(sum[:8])
+ keep := maxK8sNameLen - len(suffix)
+ return name[:keep] + suffix
+}
+
func buildCompatibilityRequirement(crd unstructured.Unstructured) (unstructured.Unstructured, error) {
@@
ObjectMeta: metav1.ObjectMeta{
- Name: compatibilityRequirementNamePrefix + crd.GetName(),
+ Name: compatibilityRequirementName(crd.GetName()),
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/revisiongenerator/compatibility.go` around lines 49 - 50, The Name field
construction at line 49 concatenates compatibilityRequirementNamePrefix with
crd.GetName() without validating the total length, which can exceed Kubernetes'
253-character DNS-subdomain limit for long CRD names. Add length validation to
ensure the resulting name stays within the limit. If the concatenated name would
be too long, either truncate the CRD name proportionally or use a hash-based
suffix of the CRD name to keep the overall name length valid and deterministic
while preventing reconciliation failures for long CRD names.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/revisiongenerator/revision_test.go`:
- Around line 701-713: The collector subtest in revision_test.go has a wsl_v5
layout violation caused by extra blank-line spacing around the local setup and
assertion. Clean up the whitespace in this test block by tightening the
separation between the collector setup, the NewRenderedRevision call, and the
final Expect assertion while keeping the same behavior. Use the surrounding
subtest body and the NewRenderedRevision/WithObjectCollectors call site to
locate and adjust the formatting.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 5c1a8ec7-c1c9-4397-8ebc-3d6d24dbf93f
📒 Files selected for processing (3)
pkg/controllers/revision/revision_controller_test.gopkg/controllers/revision/suite_test.gopkg/revisiongenerator/revision_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/controllers/revision/revision_controller_test.go
- pkg/controllers/revision/suite_test.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/controllers/installer/installer_controller_test.go (1)
129-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive
CompatibilityRequirementnames from the production builder.These literals duplicate the synthetic naming contract from the revision generator, so any future prefix/normalization change will break these tests without changing installer behavior. Please compute the expected name through the shared helper/constant instead of hard-coding
ccapio-.... As per coding guidelines, "ALWAYS check for existing patterns, and use them if found."Also applies to: 545-546, 579-580, 631-632
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/controllers/installer/installer_controller_test.go` around lines 129 - 130, The test hard-codes the synthetic CompatibilityRequirement name with the ccapio- prefix, which duplicates the naming contract and can drift from production behavior. Update the installer_controller_test assertions to derive the expected name through the same shared helper/constant used by the production builder or revision generator, instead of constructing literals like crName directly. Apply this change at all listed test cases so the expectations stay aligned with the builder’s normalization/prefix logic.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/controllers/installer/helpers_test.go`:
- Line 343: The setCompatibilityRequirementConditions helper currently takes an
admitted parameter that is never meaningfully varied, so golangci-lint flags it
as unused configurability. Either remove admitted from
setCompatibilityRequirementConditions and update all call sites that always pass
true, or add a test that exercises the admitted=false path so the parameter is
actually required.
In `@pkg/controllers/installer/installer_controller_test.go`:
- Around line 140-143: Add the missing blank line after each By(...) step in the
installer_controller_test.go suite to satisfy wsl_v5; update the affected test
blocks around the CompatibilityRequirement checks so the declarations like cr
are separated from the preceding By call, matching the surrounding test style at
the other flagged locations.
---
Nitpick comments:
In `@pkg/controllers/installer/installer_controller_test.go`:
- Around line 129-130: The test hard-codes the synthetic
CompatibilityRequirement name with the ccapio- prefix, which duplicates the
naming contract and can drift from production behavior. Update the
installer_controller_test assertions to derive the expected name through the
same shared helper/constant used by the production builder or revision
generator, instead of constructing literals like crName directly. Apply this
change at all listed test cases so the expectations stay aligned with the
builder’s normalization/prefix logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 07df3959-69ae-4722-9715-4144b4268a7d
📒 Files selected for processing (3)
pkg/controllers/installer/helpers_test.gopkg/controllers/installer/installer_controller_test.gopkg/revisiongenerator/compatibility.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/revisiongenerator/compatibility.go
theobarberbany
force-pushed
the
tb/3318
branch
2 times, most recently
from
02c5ad7 to
8865b93
Compare
2 participants
Summary by CodeRabbit