CONSOLE-5195: Add AI tooling configuration with Claude Code skills#1140
CONSOLE-5195: Add AI tooling configuration with Claude Code skills#1140TheRealJon wants to merge 1 commit intoopenshift:mainfrom
Conversation
Configure AI-assisted development and review tooling for console-operator: - Add 6 Claude Code skills (1,142 lines) for operator-specific reviews: - controller-review: Validate controller factory patterns and ManagementState - sync-handler-review: Check incremental reconciliation logic - manifest-review: Review RBAC and cluster profile annotations - e2e-test-review: Validate test patterns, cleanup, wait logic - e2e-test-create: Scaffold new e2e tests with best practices - go-quality-review: Detect deprecated APIs and code smells - Update CodeRabbit config to use pattern-based skill triggering: - Triggers based on code patterns (function signatures, types) not just file paths - More accurate detection of controller code, sync handlers, e2e tests - References all skills in knowledge base - Add comprehensive documentation: - .claude/README.md: Skills overview and usage - .github/CODERABBIT_SETUP.md: Integration setup guide Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@TheRealJon: This pull request references CONSOLE-5195 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
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: TheRealJon 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
WalkthroughIntroduced a comprehensive Claude Code integration framework for the console-operator repository, consisting of six operator-specific code review skills (controller, sync handler, manifest, Go quality, E2E test review/creation) documented under Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@TheRealJon: This pull request references CONSOLE-5195 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
.coderabbit.yaml (1)
29-31:⚠️ Potential issue | 🟠 Major
*_test.gois globally excluded, so e2e test skills won’t trigger.Line 31 (
!**/*_test.go) conflicts with the new test path instructions (Line 66+). As written, CodeRabbit won’t review test files, so/e2e-test-reviewand/e2e-test-createguidance is effectively disabled.Suggested config fix
path_filters: - "!vendor/**" - - "!**/*_test.go"Also applies to: 66-89
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.coderabbit.yaml around lines 29 - 31, The global exclusion pattern in path_filters ("!**/*_test.go") prevents any test file reviews and disables the /e2e-test-review and /e2e-test-create flows; remove or narrow that pattern in path_filters and instead either (a) delete "!**/*_test.go" and rely on the specific test includes described later, or (b) replace it with a more specific exclusion (e.g., only unit tests) and/or add explicit include patterns for your e2e test paths (e.g., "e2e/**" or the exact e2e directories) so that the e2e-test-review/e2e-test-create rules can match; update the path_filters block and keep references consistent with the existing path filters and the e2e test instructions so tests are no longer globally excluded.
🧹 Nitpick comments (1)
.claude/skills/e2e-test-review.md (1)
68-69: Prefer framework timeout constants over hardcoded durations in examples.Use
framework.AsyncOperationTimeoutin examples instead of fixed5*time.Minuteso generated tests stay aligned with the repo’s e2e framework defaults.Suggested doc fix
-err := wait.Poll(1*time.Second, 5*time.Minute, func() (bool, error) { +err := wait.Poll(1*time.Second, framework.AsyncOperationTimeout, func() (bool, error) { ... -ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) +ctx, cancel := context.WithTimeout(context.Background(), framework.AsyncOperationTimeout)Also applies to: 99-100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/e2e-test-review.md around lines 68 - 69, Replace the hardcoded timeout in the wait.Poll call with the repository's e2e framework constant: change the second argument currently set to 5*time.Minute to framework.AsyncOperationTimeout so examples use the shared AsyncOperationTimeout; update any other occurrences (e.g., the other two places noted) where 5*time.Minute is used with wait.Poll to use framework.AsyncOperationTimeout as well, keeping the wait.Poll call and callback (deployment retrieval via client.AppsV1().Deployments(ns).Get) 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 @.claude/skills/e2e-test-create.md:
- Around line 68-69: The template uses the incorrect type name "Clientset" which
should match the repo's existing type "ClientSet"; update every occurrence of
"*framework.Clientset" to "*framework.ClientSet" in the e2e test scaffold
signatures (e.g., the function parameter lists shown alongside modify
func(*operatorv1.Console)) and any other examples at the noted locations so
copy-pasted code compiles against the repository's framework package.
- Around line 103-107: Implement the cleanupResources helper to actually delete
resources in reverse creation order and ignore NotFound errors: inside
cleanupResources(ctx := context.Background(), t *testing.T, client
*framework.Clientset) iterate teardown steps in reverse (e.g., delete pods,
services, deployments, namespaces in reverse of creation), call the client
deletion methods (from framework.Clientset) and for each deletion check the
returned error and treat a NotFound error as non-fatal (log it via t.Logf) while
failing the test on other errors (t.Fatalf or t.Error); ensure the function
signature and usages reference cleanupResources, context.Background, t
*testing.T and framework.Clientset so callers remain compatible.
In @.claude/skills/go-quality-review.md:
- Around line 229-238: The "bad" example has a type mismatch: getNamespace() is
declared to return string but returns a []byte; update the function getNamespace
to return a string (e.g., return "openshift-console") or explicitly convert the
byte slice to string (e.g., return string([]byte("openshift-console"))) so the
return type matches the signature and the example compiles; prefer the simpler
constant/string literal to illustrate avoiding unnecessary allocations.
In @.claude/skills/manifest-review.md:
- Around line 54-61: The ServiceAccount example under "Service Account
References" uses `name: console` and `namespace: openshift-console`, which is
inconsistent with this repo’s RBAC pattern; update the example to reference the
operator service account used in cross-namespace bindings (e.g., `name:
console-operator` and `namespace: openshift-console-operator`) and adjust the
surrounding text to note the operator SA convention when demonstrating
`subjects:` -> `- kind: ServiceAccount` entries to avoid misleading guidance.
In @.github/CODERABBIT_SETUP.md:
- Around line 27-30: The fenced code block containing the lines
"/controller-review" and "/e2e-test-review" uses an untyped triple-backtick
fence; update that opening fence to include the language identifier "bash"
(i.e., change ``` to ```bash) so the block reads as a bash code block for
markdownlint MD040 compliance and proper rendering.
---
Outside diff comments:
In @.coderabbit.yaml:
- Around line 29-31: The global exclusion pattern in path_filters
("!**/*_test.go") prevents any test file reviews and disables the
/e2e-test-review and /e2e-test-create flows; remove or narrow that pattern in
path_filters and instead either (a) delete "!**/*_test.go" and rely on the
specific test includes described later, or (b) replace it with a more specific
exclusion (e.g., only unit tests) and/or add explicit include patterns for your
e2e test paths (e.g., "e2e/**" or the exact e2e directories) so that the
e2e-test-review/e2e-test-create rules can match; update the path_filters block
and keep references consistent with the existing path filters and the e2e test
instructions so tests are no longer globally excluded.
---
Nitpick comments:
In @.claude/skills/e2e-test-review.md:
- Around line 68-69: Replace the hardcoded timeout in the wait.Poll call with
the repository's e2e framework constant: change the second argument currently
set to 5*time.Minute to framework.AsyncOperationTimeout so examples use the
shared AsyncOperationTimeout; update any other occurrences (e.g., the other two
places noted) where 5*time.Minute is used with wait.Poll to use
framework.AsyncOperationTimeout as well, keeping the wait.Poll call and callback
(deployment retrieval via client.AppsV1().Deployments(ns).Get) 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: Pro Plus
Run ID: 91de28cb-00fc-4b44-a66a-e1211c08b9f6
📒 Files selected for processing (9)
.claude/README.md.claude/skills/controller-review.md.claude/skills/e2e-test-create.md.claude/skills/e2e-test-review.md.claude/skills/go-quality-review.md.claude/skills/manifest-review.md.claude/skills/sync-handler-review.md.coderabbit.yaml.github/CODERABBIT_SETUP.md
📜 Review details
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
URL:
File: .claude/skills/e2e-test-create.md:undefined-undefined
Timestamp: 2026-04-17T14:09:10.706Z
Learning: Test both success and failure paths in e2e tests
Learnt from: CR
URL:
File: .claude/skills/e2e-test-create.md:undefined-undefined
Timestamp: 2026-04-17T14:09:10.706Z
Learning: Verify operator responds to config changes in e2e tests
Learnt from: CR
URL:
File: .claude/skills/e2e-test-create.md:undefined-undefined
Timestamp: 2026-04-17T14:09:10.706Z
Learning: Check ClusterOperator status conditions in e2e tests
Learnt from: CR
URL:
File: .claude/skills/e2e-test-create.md:undefined-undefined
Timestamp: 2026-04-17T14:09:10.706Z
Learning: Clean up resources in reverse order of creation in e2e tests
Learnt from: CR
URL:
File: .claude/skills/sync-handler-review.md:undefined-undefined
Timestamp: 2026-04-17T14:09:49.416Z
Learning: Implement incremental sync patterns where operators use early returns on errors rather than collecting multiple errors and continuing execution
🪛 LanguageTool
.claude/README.md
[uncategorized] ~138-~138: The official name of this software platform is spelled with a capital “H”.
Context: ... repository for cross-repo context See .github/CODERABBIT_SETUP.md for complete integ...
(GITHUB)
🪛 markdownlint-cli2 (0.22.0)
.github/CODERABBIT_SETUP.md
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔀 Multi-repo context openshift/console
[::openshift/console::] .claude directory present at repository root; contains many existing skill files and settings:
- .claude/settings.json (references to CLI/version) — found in search output line: .claude/settings.json:63
- Multiple skill files under .claude/skills/ (examples found in search):
- .claude/skills/pre-push-review/SKILL.md (numerous CodeRabbit integration instructions and CLI commands) — multiple matches in search output
- .claude/skills/microcopy-review/SKILL.md — referenced in search output
- .claude/skills/update-package/SKILL.md — referenced in search output
[::openshift/console::] .coderabbit.yaml exists at repository root (search hit: .coderabbit.yaml:1) — indicates repository already integrates with CodeRabbit/skill-trigger configuration.
Implication: This repository already contains a .claude skillset and CodeRabbit configuration; adding the new Claude Code skills and updates described in the PR will extend existing tooling rather than introduce an entirely new integration surface. Recommend reviewers check .coderabbit.yaml and the .claude/skills/ directory for overlapping triggers, duplicate skill names, and CI skip patterns.
🔇 Additional comments (14)
.claude/README.md (1)
96-138: Solid integration documentation.The usage + wiring explanation is clear and matches the skill-based setup introduced in this PR.
.claude/skills/controller-review.md (1)
18-41: LGTM for controller-pattern guidance.The ManagementState/status/resourceapply checks are well scoped for this codebase.
Also applies to: 64-78
.claude/skills/sync-handler-review.md (1)
75-76: The snippet is correct as-is; bothAddConditionandAddConditionsare valid API methods.The
AddConditionmethod exists inpkg/console/status/status.go:171and is actively used throughout the codebase. The difference between the two methods is semantic:AddConditiontakes a singleConditionUpdate, whileAddConditionstakes a slice. The documented example correctly usesAddConditionfor a single condition, matching real usage patterns in the codebase (e.g.,sync_v400.go:227). No change is needed.> Likely an incorrect or invalid review comment..claude/skills/go-quality-review.md (11)
1-5: LGTM!The YAML frontmatter follows standard skill definition conventions with clear name, description, and appropriate tags.
11-42: LGTM!The deprecated API examples are accurate. The
ioutilpackage was deprecated in Go 1.16, and the modern alternatives usingos.ReadFile,os.WriteFile,io.ReadAll, and context-aware dialing are correct best practices.
44-83: LGTM!Error handling guidance is sound:
%wfor error wrapping preserves the error chain, contextual error messages improve debugging, and predicate-based error checks (e.g.,apierrors.IsNotFound) are more robust than string matching.
85-131: LGTM!Resource management examples correctly demonstrate context propagation (avoiding
context.Background()when a parent context exists) and properdeferplacement to prevent resource leaks on early returns.
133-208: LGTM!Code smell detection covers important maintainability patterns: function size limits, named constants with context, and early returns to reduce nesting. All examples demonstrate Go best practices effectively.
212-227: LGTM!The
strings.Builderexample correctly demonstrates efficient string concatenation compared to repeated string concatenation in loops.
240-262: LGTM!Concurrency example correctly demonstrates protecting shared map access with
sync.RWMutexand usingdeferfor unlock safety.
264-282: LGTM!Testing guidance correctly emphasizes checking errors in test code rather than using the blank identifier, with appropriate use of
t.Fatalffor setup errors andt.Errorffor assertion failures.
284-299: LGTM!Documentation section correctly demonstrates godoc conventions for exported functions, with the comment starting with the function name and explaining parameters.
301-308: LGTM!The output format specification is clear and well-structured, providing consistent fields (File:Line, Issue, Category, Fix, Priority) for review comments.
310-318: LGTM!Example review comments effectively demonstrate the structured output format at different priority levels with realistic scenarios.
| client *framework.Clientset, | ||
| modify func(*operatorv1.Console), |
There was a problem hiding this comment.
Template type name likely incorrect (Clientset vs ClientSet).
The examples use *framework.Clientset, but existing e2e code in this repo uses *framework.ClientSet. This will break copy-pasted scaffold output.
Suggested doc fix
- client *framework.Clientset,
+ client *framework.ClientSet,
...
- client *framework.Clientset,
+ client *framework.ClientSet,
...
-func cleanupResources(t *testing.T, client *framework.Clientset) {
+func cleanupResources(t *testing.T, client *framework.ClientSet) {
...
-func verifyExpectedState(t *testing.T, client *framework.Clientset) {
+func verifyExpectedState(t *testing.T, client *framework.ClientSet) {Also applies to: 89-90, 103-103, 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/e2e-test-create.md around lines 68 - 69, The template uses
the incorrect type name "Clientset" which should match the repo's existing type
"ClientSet"; update every occurrence of "*framework.Clientset" to
"*framework.ClientSet" in the e2e test scaffold signatures (e.g., the function
parameter lists shown alongside modify func(*operatorv1.Console)) and any other
examples at the noted locations so copy-pasted code compiles against the
repository's framework package.
| func cleanupResources(t *testing.T, client *framework.Clientset) { | ||
| ctx := context.Background() | ||
| // Clean up in reverse order of creation | ||
| // Ignore NotFound errors | ||
| } |
There was a problem hiding this comment.
Cleanup helper should explicitly model NotFound-safe teardown.
The helper is currently a placeholder and does not demonstrate the NotFound handling called out in the checklist, which weakens scaffold quality.
Suggested doc fix
func cleanupResources(t *testing.T, client *framework.ClientSet) {
- ctx := context.Background()
- // Clean up in reverse order of creation
- // Ignore NotFound errors
+ ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute)
+ defer cancel()
+ // Clean up in reverse order of creation
+ if err := client.ConsoleClient.ConsoleV1().ConsolePlugins().Delete(ctx, "test-plugin", metav1.DeleteOptions{}); err != nil && !apierrors.IsNotFound(err) {
+ t.Logf("cleanup warning: failed deleting ConsolePlugin: %v", err)
+ }
}Based on learnings: Clean up resources in reverse order of creation in e2e tests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/e2e-test-create.md around lines 103 - 107, Implement the
cleanupResources helper to actually delete resources in reverse creation order
and ignore NotFound errors: inside cleanupResources(ctx := context.Background(),
t *testing.T, client *framework.Clientset) iterate teardown steps in reverse
(e.g., delete pods, services, deployments, namespaces in reverse of creation),
call the client deletion methods (from framework.Clientset) and for each
deletion check the returned error and treat a NotFound error as non-fatal (log
it via t.Logf) while failing the test on other errors (t.Fatalf or t.Error);
ensure the function signature and usages reference cleanupResources,
context.Background, t *testing.T and framework.Clientset so callers remain
compatible.
| ### Unnecessary Allocations | ||
| ```go | ||
| // Bad - allocates on every call | ||
| func (c *Controller) getNamespace() string { | ||
| return []byte("openshift-console")[0:] // Unnecessary allocation | ||
| } | ||
|
|
||
| // Good - use constant | ||
| const namespace = "openshift-console" | ||
| ``` |
There was a problem hiding this comment.
Fix the syntax error in the "bad" example.
The function signature declares a string return type, but line 233 returns []byte("openshift-console")[0:], which is a []byte. This is a compile error.
🐛 Proposed fix for the example
A more realistic example of unnecessary allocation might be:
-// Bad - allocates on every call
-func (c *Controller) getNamespace() string {
- return []byte("openshift-console")[0:] // Unnecessary allocation
-}
+// Bad - unnecessary conversion allocates on every call
+func (c *Controller) getNamespace() string {
+ return string([]byte("openshift-console"))
+}
// Good - use constant
const namespace = "openshift-console"Or even simpler:
-// Bad - allocates on every call
-func (c *Controller) getNamespace() string {
- return []byte("openshift-console")[0:] // Unnecessary allocation
-}
+// Bad - recomputes on every call when it could be a constant
+func (c *Controller) getNamespace() string {
+ return "openshift-" + "console"
+}
// Good - use constant
const namespace = "openshift-console"📝 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.
| ### Unnecessary Allocations | |
| ```go | |
| // Bad - allocates on every call | |
| func (c *Controller) getNamespace() string { | |
| return []byte("openshift-console")[0:] // Unnecessary allocation | |
| } | |
| // Good - use constant | |
| const namespace = "openshift-console" | |
| ``` | |
| ### Unnecessary Allocations |
| ### Unnecessary Allocations | |
| ```go | |
| // Bad - allocates on every call | |
| func (c *Controller) getNamespace() string { | |
| return []byte("openshift-console")[0:] // Unnecessary allocation | |
| } | |
| // Good - use constant | |
| const namespace = "openshift-console" | |
| ``` | |
| ### Unnecessary Allocations |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/go-quality-review.md around lines 229 - 238, The "bad"
example has a type mismatch: getNamespace() is declared to return string but
returns a []byte; update the function getNamespace to return a string (e.g.,
return "openshift-console") or explicitly convert the byte slice to string
(e.g., return string([]byte("openshift-console"))) so the return type matches
the signature and the example compiles; prefer the simpler constant/string
literal to illustrate avoiding unnecessary allocations.
| ### 6. Service Account References | ||
| When binding roles to service accounts: | ||
| ```yaml | ||
| subjects: | ||
| - kind: ServiceAccount | ||
| name: console | ||
| namespace: openshift-console | ||
| ``` |
There was a problem hiding this comment.
ServiceAccount example is inconsistent with this repo’s RBAC pattern.
The example suggests console in openshift-console, but this repo’s role bindings commonly grant permissions to the operator SA (console-operator) from openshift-console-operator in cross-namespace scenarios. This can generate false-positive review guidance.
Suggested doc fix
subjects:
- kind: ServiceAccount
- name: console
- namespace: openshift-console
+ name: console-operator
+ namespace: openshift-console-operator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/manifest-review.md around lines 54 - 61, The ServiceAccount
example under "Service Account References" uses `name: console` and `namespace:
openshift-console`, which is inconsistent with this repo’s RBAC pattern; update
the example to reference the operator service account used in cross-namespace
bindings (e.g., `name: console-operator` and `namespace:
openshift-console-operator`) and adjust the surrounding text to note the
operator SA convention when demonstrating `subjects:` -> `- kind:
ServiceAccount` entries to avoid misleading guidance.
| ``` | ||
| /controller-review | ||
| /e2e-test-review | ||
| ``` |
There was a problem hiding this comment.
Add language identifier to fenced code block (markdownlint MD040).
Line 27 uses an untyped fence; add bash for lint compliance and better rendering.
Suggested doc fix
-```
+```bash
/controller-review
/e2e-test-review</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **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.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 27-27: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/CODERABBIT_SETUP.md around lines 27 - 30, The fenced code block
containing the lines "/controller-review" and "/e2e-test-review" uses an untyped
triple-backtick fence; update that opening fence to include the language
identifier "bash" (i.e., change ``` to ```bash) so the block reads as a bash
code block for markdownlint MD040 compliance and proper rendering.
2 participants
Configure AI-assisted development and review tooling for console-operator:
Add 6 Claude Code skills (1,142 lines) for operator-specific reviews:
Update CodeRabbit config to use pattern-based skill triggering:
Add comprehensive documentation:
Summary by CodeRabbit
Documentation
Chores