NO-ISSUE: Add claude md file for using the node_utils and export util functions#31078
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
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:
WalkthroughExports many previously unexported node test helpers in ChangesNode utilities export, tests wiring, docs, and validation
Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
|
/assign @harche |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/.claude-docs-check.sh`:
- Around line 11-17: The parser for exported functions (UTILS_FUNCS) is too
narrow and the documentation lookup uses substring matching, so update the
extractor and matcher: change the grep/sed pipeline that builds UTILS_FUNCS to
accept optional receiver syntax and digits (e.g., use a regex like '^func(
\([^)]+\) )?[A-Z][A-Za-z0-9_]*' and extract the actual identifier), and replace
the doc existence check (the grep -q "$func" "$CLAUDE_MD") with a word-boundary
or anchoring regex (e.g., grep -q -E "(^|[^A-Za-z0-9_])$func([^A-Za-z0-9_]|$)")
so method names like Exec2 and receiver methods like (c *Client) Exec2 are
discovered and false substring matches are avoided; keep references to
UTILS_FUNCS, NODE_UTILS, CLAUDE_MD, and the for-loop variable func when making
the edits.
In `@test/extended/node/CLAUDE.md`:
- Around line 150-157: After filtering with getPureWorkerNodes, workerNodes may
be empty and the direct access workerNodes[0] can panic; update the test to
check len(workerNodes) before using index 0 (e.g., assert
o.Expect(len(workerNodes)).To(o.BeNumerically(">", 0)) or handle the zero case
by skipping/failing the test) and only set nodeName from workerNodes[0].Name
after that check so getNodesByLabel, getPureWorkerNodes and the workerNodes ->
nodeName usage are safe.
🪄 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: 4241d8cc-6825-48e9-8089-7dd5e78638da
📒 Files selected for processing (3)
test/extended/node/.claude-docs-check.shtest/extended/node/CLAUDE.mdtest/extended/node/node_utils.go
|
Scheduling required tests: |
harche
left a comment
harche
left a comment
There was a problem hiding this comment.
Overall solid idea — documenting utility functions to reduce duplication is valuable. A few issues to address before merging.
ngopalak-redhat
force-pushed
the
ngopalak/util_function_node_help_claude
branch
from
8ffee6a to
d13dcc7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/claude-docs-check.sh`:
- Around line 9-14: The current UTILS_FUNCS extraction only matches capitalized
(exported) functions so it misses lowercase helpers in node_utils.go; update the
extraction logic used in UTILS_FUNCS (and the identical pattern referenced
around lines 22-26) to allow both lowercase and uppercase function names by
changing the name-matching character class (replace the [A-Z] constraint) so the
grep/sed regex captures names starting with any letter (e.g., [A-Za-z] or
[a-zA-Z]) and thus includes unexported helpers, or alternatively restrict the
docs to exported helpers only if you intend to keep the original behavior.
In `@test/extended/node/CLAUDE.md`:
- Around line 132-133: Update the guidance sentence that currently states “All
utility functions and test functions should use `context.Context`” to a clearer
instruction such as “Always pass `ctx` when the helper or test function
signature requires it (use `context.Context` where supported).” Also update the
documented helpers in this section to either accept `ctx` in their examples or
call out explicitly which helpers do not support `ctx`, ensuring examples and
text use the `ctx` variable consistently so readers are not misled by
conflicting signatures.
🪄 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: da983fe4-cc13-456b-9474-1f042a9192bb
📒 Files selected for processing (3)
test/extended/node/CLAUDE.mdtest/extended/node/claude-docs-check.shtest/extended/node/node_utils.go
✅ Files skipped from review due to trivial changes (1)
- test/extended/node/node_utils.go
ngopalak-redhat
force-pushed
the
ngopalak/util_function_node_help_claude
branch
from
d13dcc7 to
b16756e
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/extended/node/CLAUDE.md (1)
1-189:⚠️ Potential issue | 🟡 MinorIntegrate the consistency check script into CI to enforce documentation updates automatically.
The
claude-docs-check.shscript exists and correctly validates that exported functions fromnode_utils.goare documented inCLAUDE.md. However, it is not currently referenced in any CI/Makefile configuration, so the check won't run automatically on pull requests. Add this validation to your CI pipeline (e.g., Makefile, GitHub Actions, or OpenShift CI) to prevent documentation drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/CLAUDE.md` around lines 1 - 189, The CI is missing an enforcement step: the claude-docs-check.sh script that verifies exported functions in node_utils.go are documented in CLAUDE.md is not invoked by CI. Add a build target (e.g., "docs-check") that runs claude-docs-check.sh and fails on non-zero exit, then wire that target into your CI pipeline (e.g., add it as a pre-merge step in the GitHub Actions workflow or OpenShift CI presubmit job) so the script runs on PRs and blocks merges when documentation is out of sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@test/extended/node/CLAUDE.md`:
- Around line 1-189: The CI is missing an enforcement step: the
claude-docs-check.sh script that verifies exported functions in node_utils.go
are documented in CLAUDE.md is not invoked by CI. Add a build target (e.g.,
"docs-check") that runs claude-docs-check.sh and fails on non-zero exit, then
wire that target into your CI pipeline (e.g., add it as a pre-merge step in the
GitHub Actions workflow or OpenShift CI presubmit job) so the script runs on PRs
and blocks merges when documentation is out of sync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: a7fa5ddf-4478-47f8-88bf-64d6762f4db5
📒 Files selected for processing (3)
test/extended/node/CLAUDE.mdtest/extended/node/claude-docs-check.shtest/extended/node/node_utils.go
✅ Files skipped from review due to trivial changes (1)
- test/extended/node/node_utils.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/extended/node/claude-docs-check.sh
ngopalak-redhat
force-pushed
the
ngopalak/util_function_node_help_claude
branch
from
b16756e to
e7dad96
Compare
ngopalak-redhat
changed the title
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 (1)
test/extended/node/node_utils.go (1)
575-587:⚠️ Potential issue | 🟠 MajorParse numeric suffix when choosing the “highest numbered” worker kubelet MachineConfig
At Line 584, comparing names lexicographically can select the wrong config (e.g.,
"…-9"vs"…-10"). This can make downstream validations read the wrong MachineConfig.Proposed fix
func GetWorkerGeneratedKubeletMC(ctx context.Context, mcClient *machineconfigclient.Clientset) (*machineconfigv1.MachineConfig, error) { mcList, err := mcClient.MachineconfigurationV1().MachineConfigs().List(ctx, metav1.ListOptions{}) if err != nil { return nil, err } var highestMC *machineconfigv1.MachineConfig + highestSuffix := -1 for i := range mcList.Items { - if strings.Contains(mcList.Items[i].Name, "worker-generated-kubelet") { - if highestMC == nil || mcList.Items[i].Name > highestMC.Name { - highestMC = &mcList.Items[i] - } - } + name := mcList.Items[i].Name + if !strings.Contains(name, "worker-generated-kubelet") { + continue + } + parts := strings.Split(name, "-") + if len(parts) == 0 { + continue + } + suffix, convErr := strconv.Atoi(parts[len(parts)-1]) + if convErr != nil { + continue + } + if suffix > highestSuffix { + highestSuffix = suffix + highestMC = &mcList.Items[i] + } }// add import import "strconv"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/node_utils.go` around lines 575 - 587, GetWorkerGeneratedKubeletMC currently picks the "highest" worker-generated-kubelet MachineConfig by lexicographic name comparison which fails for numeric suffixes (e.g., "-9" vs "-10"); change the selection logic in GetWorkerGeneratedKubeletMC to parse the numeric suffix of mcList.Items[i].Name (use strconv.Atoi on the portion after the final dash) and compare integers to pick the largest suffix, falling back to name string comparison if parsing fails; update imports to include "strconv" and ensure highestMC is updated based on the numeric comparison rather than raw string ordering.
♻️ Duplicate comments (1)
test/extended/node/CLAUDE.md (1)
159-159:⚠️ Potential issue | 🟡 MinorAlign Line 159 with mixed helper signatures
Line 159 currently overstates context usage and conflicts with the note at Line 97. Reword to “pass
ctxwhen the helper signature requires it.”Proposed fix
-3. **Don't skip context propagation** - always pass `ctx` to utility functions +3. **Don't skip context propagation** - pass `ctx` to helpers that accept `context.Context`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/CLAUDE.md` at line 159, Update the bullet titled "Don't skip context propagation" so the guidance reads "pass `ctx` when the helper signature requires it." Replace the current explicit "always pass `ctx` to utility functions" wording to avoid conflict with the earlier note about mixed helper signatures; locate and edit the markdown paragraph under that bullet (the heading text "Don't skip context propagation") to use the new phrasing.
🧹 Nitpick comments (1)
test/extended/node/node_swap_cnv.go (1)
162-163: UseGetControlPlaneNodesin TC1 for label-agnostic control-plane discoveryLine 162 hardcodes the
masterlabel and can miss clusters that only exposecontrol-plane. ReuseGetControlPlaneNodeshere (as done later in this file).Proposed fix
- controlPlaneNodes, err := GetNodesByLabel(ctx, oc, "node-role.kubernetes.io/master") + controlPlaneNodes, err := GetControlPlaneNodes(ctx, oc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/node/node_swap_cnv.go` around lines 162 - 163, The call to GetNodesByLabel with the "node-role.kubernetes.io/master" label in TC1 should be replaced with GetControlPlaneNodes to discover control-plane nodes in a label-agnostic way; locate the code that sets controlPlaneNodes (currently using GetNodesByLabel) and call GetControlPlaneNodes(ctx, oc) instead, keep the same error check that asserts err is nil (o.Expect(err).NotTo(o.HaveOccurred())), and ensure any downstream use of controlPlaneNodes remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/node_utils.go`:
- Around line 273-279: The helper CleanupDropInAndRestartKubelet currently
swallows errors from RemoveDropInFile and RestartKubeletOnNode; change
CleanupDropInAndRestartKubelet to propagate errors (return error) and capture
and handle the return values from RemoveDropInFile and RestartKubeletOnNode
(e.g., if err != nil log and return err) before proceeding to
WaitForNodeToBeReady; update callers to handle the returned error. Ensure you
reference the functions RemoveDropInFile, RestartKubeletOnNode,
WaitForNodeToBeReady and the helper CleanupDropInAndRestartKubelet when making
the change.
---
Outside diff comments:
In `@test/extended/node/node_utils.go`:
- Around line 575-587: GetWorkerGeneratedKubeletMC currently picks the "highest"
worker-generated-kubelet MachineConfig by lexicographic name comparison which
fails for numeric suffixes (e.g., "-9" vs "-10"); change the selection logic in
GetWorkerGeneratedKubeletMC to parse the numeric suffix of mcList.Items[i].Name
(use strconv.Atoi on the portion after the final dash) and compare integers to
pick the largest suffix, falling back to name string comparison if parsing
fails; update imports to include "strconv" and ensure highestMC is updated based
on the numeric comparison rather than raw string ordering.
---
Duplicate comments:
In `@test/extended/node/CLAUDE.md`:
- Line 159: Update the bullet titled "Don't skip context propagation" so the
guidance reads "pass `ctx` when the helper signature requires it." Replace the
current explicit "always pass `ctx` to utility functions" wording to avoid
conflict with the earlier note about mixed helper signatures; locate and edit
the markdown paragraph under that bullet (the heading text "Don't skip context
propagation") to use the new phrasing.
---
Nitpick comments:
In `@test/extended/node/node_swap_cnv.go`:
- Around line 162-163: The call to GetNodesByLabel with the
"node-role.kubernetes.io/master" label in TC1 should be replaced with
GetControlPlaneNodes to discover control-plane nodes in a label-agnostic way;
locate the code that sets controlPlaneNodes (currently using GetNodesByLabel)
and call GetControlPlaneNodes(ctx, oc) instead, keep the same error check that
asserts err is nil (o.Expect(err).NotTo(o.HaveOccurred())), and ensure any
downstream use of controlPlaneNodes remains unchanged.
🪄 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: 2816d278-298b-49fe-8586-d989da75a542
📒 Files selected for processing (7)
test/extended/node/CLAUDE.mdtest/extended/node/claude-docs-check.shtest/extended/node/node_sizing.gotest/extended/node/node_swap.gotest/extended/node/node_swap_cnv.gotest/extended/node/node_utils.gotest/extended/node/system_compressible.go
✅ Files skipped from review due to trivial changes (1)
- test/extended/node/claude-docs-check.sh
ngopalak-redhat
changed the title
|
@ngopalak-redhat: This pull request explicitly references no jira issue. 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
|
/payload-job periodic-ci-openshift-release-main-nightly-4.22-e2e-aws-disruptive-longrunning |
|
@ngopalak-redhat: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/24dc41b0-42be-11f1-9810-f2840d26fc7d-0 |
|
/assign @cpmeadors |
ngopalak-redhat
changed the title
|
Scheduling required tests: |
openshift-merge-bot
Bot
added
the
ready-for-human-review
|
@harche All the tests I have modified have passed: https://pr-payload-tests.ci.openshift.org/runs/ci/24dc41b0-42be-11f1-9810-f2840d26fc7d-0 |
ngopalak-redhat
force-pushed
the
ngopalak/util_function_node_help_claude
branch
from
e7dad96 to
39e606e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/node/CLAUDE.md`:
- Line 17: Update the guidance to avoid an absolute rule: change the line
referencing ExecOnNodeWithChroot so it recommends using ExecOnNodeWithChroot for
most root-level commands inside a debug container but explicitly allow
exceptions (e.g., swap management and other cases requiring namespace entry)
where ExecOnNodeWithNsenter should be used; reference the symbols
ExecOnNodeWithChroot and ExecOnNodeWithNsenter in the wording so readers know
which function to choose.
🪄 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: 1b672906-4eb9-49e1-8043-c114a335c100
📒 Files selected for processing (7)
test/extended/node/CLAUDE.mdtest/extended/node/claude-docs-check.shtest/extended/node/node_sizing.gotest/extended/node/node_swap.gotest/extended/node/node_swap_cnv.gotest/extended/node/node_utils.gotest/extended/node/system_compressible.go
✅ Files skipped from review due to trivial changes (4)
- test/extended/node/claude-docs-check.sh
- test/extended/node/system_compressible.go
- test/extended/node/node_sizing.go
- test/extended/node/node_swap_cnv.go
🚧 Files skipped from review as they are similar to previous changes (2)
- test/extended/node/node_swap.go
- test/extended/node/node_utils.go
|
@harche I have update the Claude.md file to have the human knowledge for some of the functions. Removed generated text. It can further improved when more utility functions are added. |
|
Scheduling required tests: |
|
@ngopalak-redhat: 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cpmeadors, ngopalak-redhat The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
4 participants
Adds Claude.md file so that the node_utils functions are used by the team when developing test cases with Claude.
Currently I see some duplicate code and this is one way to reduce it.
I have to refactor the functions to use capital letter to start so that they look like the utility functions and other can notice it and use it.
Later we can add some github action or makefile change that will automatically keep the Claude.md file updated.
Summary by CodeRabbit
Tests
Documentation