chore(SREP-4482, SREP-4486, SREP-4800: Boilerplate Update for Agentic SDLC Rollout)#239
chore(SREP-4482, SREP-4486, SREP-4800: Boilerplate Update for Agentic SDLC Rollout)#239charlesgong wants to merge 4 commits intoopenshift:mainfrom
Conversation
Adds .pre-commit-config.yaml with Tier 1 common hooks mirroring ci/prow/lint. Golden rules: SREP-4450 Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Addresses staticcheck QF1012: WriteString(fmt.Sprintf(...)) should be replaced with fmt.Fprintf for clarity and efficiency. Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@charlesgong: This pull request references SREP-4482 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. This pull request references SREP-4486 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. This pull request references SREP-4800 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "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. |
openshift-ci-robot
added
the
jira/valid-reference
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: charlesgong 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 |
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (2)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR adds context propagation to Kubernetes client helper functions, updates base image tags across CI/build configuration, establishes pre-commit hook infrastructure with security and linting checks, adjusts coverage thresholds, and removes a user from the owners list. ChangesContext Propagation Refactor
Infrastructure & Configuration Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (9 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #239 +/- ##
=======================================
Coverage 58.60% 58.60%
=======================================
Files 5 5
Lines 244 244
=======================================
Hits 143 143
Misses 87 87
Partials 14 14 🚀 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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controllers/machineset_controller.go (1)
142-160:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
getExpectedLabelsmutates MachineSet template labels by reference.At Line 143,
result := machineSet.Spec.Template.Spec.Labelsaliases the original map, and Line 159 deletes keys from it. This leaks side effects intomachineSetand subsequent reconciliation logic.Proposed fix
func (r *MachinesetReconciler) getExpectedLabels(machineSet *machinev1beta1.MachineSet, machine *machinev1beta1.Machine, node *corev1.Node) map[string]string { - result := machineSet.Spec.Template.Spec.Labels + result := make(map[string]string, len(machineSet.Spec.Template.Spec.Labels)) + for k, v := range machineSet.Spec.Template.Spec.Labels { + result[k] = v + }🤖 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 `@controllers/machineset_controller.go` around lines 142 - 160, The function getExpectedLabels currently aliases machineSet.Spec.Template.Spec.Labels into result and then deletes keys, mutating the MachineSet's template; instead, create a new map (e.g., newResult) and copy all entries from machineSet.Spec.Template.Spec.Labels into it (handle nil by initializing an empty map) and then perform the delete operations on that new map; return the new map so the original machineSet.Spec.Template.Spec.Labels is never modified in-place (refer to symbols: getExpectedLabels, result, machineSet.Spec.Template.Spec.Labels, delete(result,...)).
🤖 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 @.claude/commands/pre-commit.md:
- Around line 77-85: The fenced code block showing the "PRE-COMMIT SUMMARY" in
.claude/commands/pre-commit.md lacks a language hint which triggers markdownlint
MD040; update the opening fence for that block (the block containing the
"PRE-COMMIT SUMMARY" header and list) to include a language hint (e.g., change
the opening ``` to ```text) so the fenced summary block is annotated and the
lint rule is satisfied.
In `@controllers/machineset_controller_test.go`:
- Around line 45-50: The current test setup swallows errors from
corev1.AddToScheme(s) and machinev1beta1.AddToScheme(s) by only printing
messages; change those branches to fail the test immediately by propagating the
error (e.g., call panic(fmt.Sprintf(..., err)) or use the test harness' fatal
helper such as t.Fatalf("failed adding apis to scheme: %v", err) if a *testing.T
is available). Update the error handling for the AddToScheme calls
(corev1.AddToScheme and machinev1beta1.AddToScheme) to include the actual err
and abort so the suite cannot continue with a bad scheme.
In `@pkg/machine/machine.go`:
- Around line 85-88: GetNodeForMachine currently dereferences
m.Status.NodeRef.Name without checking for nil; add a guard at the top of
GetNodeForMachine that checks if m == nil or m.Status.NodeRef == nil and return
a clear error (or nil, error) when NodeRef is missing instead of proceeding to
c.Get; this prevents panics and keeps the existing client.Get call using
m.Status.NodeRef.Name only after confirming NodeRef is non-nil.
---
Outside diff comments:
In `@controllers/machineset_controller.go`:
- Around line 142-160: The function getExpectedLabels currently aliases
machineSet.Spec.Template.Spec.Labels into result and then deletes keys, mutating
the MachineSet's template; instead, create a new map (e.g., newResult) and copy
all entries from machineSet.Spec.Template.Spec.Labels into it (handle nil by
initializing an empty map) and then perform the delete operations on that new
map; return the new map so the original machineSet.Spec.Template.Spec.Labels is
never modified in-place (refer to symbols: getExpectedLabels, result,
machineSet.Spec.Template.Spec.Labels, delete(result,...)).
🪄 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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f43a3d82-52fd-471a-9503-7f20387be318
⛔ Files ignored due to path filters (8)
boilerplate/_data/backing-image-tagis excluded by!boilerplate/**boilerplate/_data/last-boilerplate-commitis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/.codecov.ymlis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/OWNERS_ALIASESis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/golangci.ymlis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/pre-commit-config.yamlis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/standard.mkis excluded by!boilerplate/**boilerplate/openshift/golang-osd-operator/updateis excluded by!boilerplate/**
📒 Files selected for processing (15)
.ci-operator.yaml.claude/commands/pre-commit.md.codecov.yml.gitignore.pre-commit-config.yamlOWNERS_ALIASESbuild/Dockerfilebuild/Dockerfile.olm-registrycontrollers/machineset_controller.gocontrollers/machineset_controller_test.gofips.goint/int_test.gopkg/machine/machine.gopkg/machine/machine_test.gopkg/metrics/metrics_test.go
💤 Files with no reviewable changes (1)
- OWNERS_ALIASES
| ``` | ||
| PRE-COMMIT SUMMARY | ||
| ================== | ||
| Passed: <list of hook IDs> | ||
| Auto-fixed: <list of hook IDs> → files staged | ||
| Fixed: <list of hook IDs> → changes applied | ||
| Failed: <list of hook IDs> → escalated to human | ||
| Attempts: <N> of 2 maximum | ||
| ``` |
There was a problem hiding this comment.
Add a language hint to the fenced summary block (markdownlint MD040).
Line 77 uses a fenced code block without a language, which triggers lint and can fail docs checks.
Suggested patch
-```
+```text
PRE-COMMIT SUMMARY
==================
Passed: <list of hook IDs>
Auto-fixed: <list of hook IDs> → files staged
Fixed: <list of hook IDs> → changes applied
Failed: <list of hook IDs> → escalated to human
Attempts: <N> of 2 maximum</details>
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.22.1)</summary>
[warning] 77-77: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.claude/commands/pre-commit.md around lines 77 - 85, The fenced code block
showing the "PRE-COMMIT SUMMARY" in .claude/commands/pre-commit.md lacks a
language hint which triggers markdownlint MD040; update the opening fence for
that block (the block containing the "PRE-COMMIT SUMMARY" header and list) to
include a language hint (e.g., change the opening totext) so the fenced
summary block is annotated and the lint rule is satisfied.
</details>
<!-- fingerprinting:phantom:poseidon:hawk -->
<!-- d98c2f50 -->
<!-- This is an auto-generated comment by CodeRabbit -->
| if err := corev1.AddToScheme(s); err != nil { | ||
| fmt.Printf("failed adding apis to scheme in machineset controller tests") | ||
| _, _ = fmt.Printf("failed adding apis to scheme in machineset controller tests") | ||
| } | ||
| if err := machinev1beta1.AddToScheme(s); err != nil { | ||
| fmt.Printf("failed adding apis to scheme in machineset controller tests") | ||
| _, _ = fmt.Printf("failed adding apis to scheme in machineset controller tests") | ||
| } |
There was a problem hiding this comment.
Fail fast when AddToScheme fails in test setup.
At Line 46 and Line 49, setup errors are only printed. These should fail the suite immediately to avoid false/opaque downstream test failures.
Proposed fix
if err := corev1.AddToScheme(s); err != nil {
- _, _ = fmt.Printf("failed adding apis to scheme in machineset controller tests")
+ Fail(fmt.Sprintf("failed adding core APIs to scheme in machineset controller tests: %v", err))
}
if err := machinev1beta1.AddToScheme(s); err != nil {
- _, _ = fmt.Printf("failed adding apis to scheme in machineset controller tests")
+ Fail(fmt.Sprintf("failed adding machine APIs to scheme in machineset controller tests: %v", err))
}📝 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.
| if err := corev1.AddToScheme(s); err != nil { | |
| fmt.Printf("failed adding apis to scheme in machineset controller tests") | |
| _, _ = fmt.Printf("failed adding apis to scheme in machineset controller tests") | |
| } | |
| if err := machinev1beta1.AddToScheme(s); err != nil { | |
| fmt.Printf("failed adding apis to scheme in machineset controller tests") | |
| _, _ = fmt.Printf("failed adding apis to scheme in machineset controller tests") | |
| } | |
| if err := corev1.AddToScheme(s); err != nil { | |
| Fail(fmt.Sprintf("failed adding core APIs to scheme in machineset controller tests: %v", err)) | |
| } | |
| if err := machinev1beta1.AddToScheme(s); err != nil { | |
| Fail(fmt.Sprintf("failed adding machine APIs to scheme in machineset controller tests: %v", err)) | |
| } |
🤖 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 `@controllers/machineset_controller_test.go` around lines 45 - 50, The current
test setup swallows errors from corev1.AddToScheme(s) and
machinev1beta1.AddToScheme(s) by only printing messages; change those branches
to fail the test immediately by propagating the error (e.g., call
panic(fmt.Sprintf(..., err)) or use the test harness' fatal helper such as
t.Fatalf("failed adding apis to scheme: %v", err) if a *testing.T is available).
Update the error handling for the AddToScheme calls (corev1.AddToScheme and
machinev1beta1.AddToScheme) to include the actual err and abort so the suite
cannot continue with a bad scheme.
| func GetNodeForMachine(ctx context.Context, c client.Client, m *machinev1.Machine) (*corev1.Node, error) { | ||
| node := &corev1.Node{} | ||
| err := c.Get(context.TODO(), types.NamespacedName{Name: m.Status.NodeRef.Name}, node) | ||
| err := c.Get(ctx, types.NamespacedName{Name: m.Status.NodeRef.Name}, node) | ||
| if err != nil { |
There was a problem hiding this comment.
Guard NodeRef before dereferencing to avoid panic.
At Line 87, m.Status.NodeRef.Name is accessed without checking m.Status.NodeRef != nil. This can panic on machines missing NodeRef.
Proposed fix
func GetNodeForMachine(ctx context.Context, c client.Client, m *machinev1.Machine) (*corev1.Node, error) {
+ if m == nil || m.Status.NodeRef == nil || m.Status.NodeRef.Name == "" {
+ return &corev1.Node{}, fmt.Errorf("machine has no node reference")
+ }
node := &corev1.Node{}
err := c.Get(ctx, types.NamespacedName{Name: m.Status.NodeRef.Name}, node)
if err != nil {
return &corev1.Node{}, err
}
return node, err
}📝 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.
| func GetNodeForMachine(ctx context.Context, c client.Client, m *machinev1.Machine) (*corev1.Node, error) { | |
| node := &corev1.Node{} | |
| err := c.Get(context.TODO(), types.NamespacedName{Name: m.Status.NodeRef.Name}, node) | |
| err := c.Get(ctx, types.NamespacedName{Name: m.Status.NodeRef.Name}, node) | |
| if err != nil { | |
| func GetNodeForMachine(ctx context.Context, c client.Client, m *machinev1.Machine) (*corev1.Node, error) { | |
| if m == nil || m.Status.NodeRef == nil || m.Status.NodeRef.Name == "" { | |
| return &corev1.Node{}, fmt.Errorf("machine has no node reference") | |
| } | |
| node := &corev1.Node{} | |
| err := c.Get(ctx, types.NamespacedName{Name: m.Status.NodeRef.Name}, node) | |
| if err != nil { | |
| return &corev1.Node{}, err | |
| } | |
| return node, err | |
| } |
🤖 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/machine/machine.go` around lines 85 - 88, GetNodeForMachine currently
dereferences m.Status.NodeRef.Name without checking for nil; add a guard at the
top of GetNodeForMachine that checks if m == nil or m.Status.NodeRef == nil and
return a clear error (or nil, error) when NodeRef is missing instead of
proceeding to c.Get; this prevents panics and keeps the existing client.Get call
using m.Status.NodeRef.Name only after confirming NodeRef is non-nil.
Co-Authored-By: Claude Sonnet 4.6 (1M context) <noreply@anthropic.com>
|
@charlesgong: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
What type of PR is this?
boilerplate
What this PR does / why we need it?
This PR moves the changes introduced in boilerplate for Agentic SDLC Rollout into MVP for ocm-agent-operator.
Related BP MRs
Which Jira/Github issue(s) this PR fixes?
Part of Rollout for Agentic SDLC -
Special notes for your reviewer:
Pre-checks (if applicable):
Summary by CodeRabbit
Chores
Refactor