From bf7c7969a4937f12519983d4051c99d42b03fc31 Mon Sep 17 00:00:00 2001 From: Jon Jackson Date: Wed, 13 May 2026 12:58:46 -0400 Subject: [PATCH] CONSOLE-5195: Add AI tooling configuration with Claude Code skills Add CodeRabbit integration and Claude Code skills for operator-specific code review patterns. Skills added: - controller-review: Review controller factory patterns and status handling - sync-handler-review: Review incremental reconciliation patterns - manifest-review: Review RBAC and cluster profile annotations - unit-test-review: Review table-driven tests and deep equality - e2e-test-review: Review e2e framework patterns and wait/retry logic - go-quality-review: Review deprecated APIs and code quality CodeRabbit configuration applies skills based on code patterns (function signatures, types) rather than file paths for more reliable reviews. Co-Authored-By: Claude Sonnet 4.5 --- .claude/skills/controller-review.md | 121 ++++++++++ .claude/skills/e2e-test-review.md | 279 ++++++++++++++++++++++ .claude/skills/go-quality-review.md | 318 ++++++++++++++++++++++++++ .claude/skills/manifest-review.md | 102 +++++++++ .claude/skills/sync-handler-review.md | 151 ++++++++++++ .claude/skills/unit-test-review.md | 301 ++++++++++++++++++++++++ .coderabbit.yaml | 129 +++++++++-- .github/CODERABBIT_SETUP.md | 130 +++++++++++ 8 files changed, 1509 insertions(+), 22 deletions(-) create mode 100644 .claude/skills/controller-review.md create mode 100644 .claude/skills/e2e-test-review.md create mode 100644 .claude/skills/go-quality-review.md create mode 100644 .claude/skills/manifest-review.md create mode 100644 .claude/skills/sync-handler-review.md create mode 100644 .claude/skills/unit-test-review.md create mode 100644 .github/CODERABBIT_SETUP.md diff --git a/.claude/skills/controller-review.md b/.claude/skills/controller-review.md new file mode 100644 index 000000000..d998eb7d1 --- /dev/null +++ b/.claude/skills/controller-review.md @@ -0,0 +1,121 @@ +--- +name: controller-review +description: Review controller code for OpenShift operator patterns and library-go conventions +tags: [review, controller, operator] +--- + +# Controller Review Skill + +Review controller implementation code following OpenShift console-operator patterns. + +## What to Check + +### 1. Controller Factory Pattern +- Uses `factory.New().WithFilteredEventsInformers()` pattern +- Proper informer filtering with `util.IncludeNamesFilter()` +- ToController() call with descriptive name and recorder + +### 2. ManagementState Handling +All controllers should handle three states: +```go +switch operatorConfig.Spec.ManagementState { +case operatorv1.Managed: + // sync logic +case operatorv1.Unmanaged: + // skip sync, return nil +case operatorv1.Removed: + // cleanup/removal logic +default: + return fmt.Errorf("unknown state: %v", ...) +} +``` + +### 3. Status Condition Handling +- Use `status.NewStatusHandler(c.operatorClient)` +- Set conditions with appropriate types: + - `status.HandleProgressingOrDegraded()` for transient errors + - `status.HandleDegraded()` for degraded conditions + - `status.HandleProgressing()` for in-progress state + - `status.HandleAvailable()` for availability +- Always `FlushAndReturn()` at end of sync + +### 4. Import Organization +Imports must be grouped with comments: +```go +import ( + // standard lib + "context" + "fmt" + + // 3rd party + "github.com/spf13/cobra" + + // kube + "k8s.io/client-go/kubernetes" + + // openshift + "github.com/openshift/api/config/v1" + + // operator (internal) + "github.com/openshift/console-operator/pkg/api" +) +``` + +### 5. Error Handling +- Wrap errors with context: `fmt.Errorf("failed to X: %w", err)` +- Check for `apierrors.IsNotFound()` when appropriate: + - **Delete operations** - NotFound means already deleted (success): + ```go + err := c.client.Delete(ctx, name, metav1.DeleteOptions{}) + if apierrors.IsNotFound(err) { + return nil // Already deleted, success + } + return err + ``` + - **Optional resources** - Resource might not exist yet: + ```go + config, err := c.lister.Get(name) + if err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("failed to get config: %w", err) + } + if apierrors.IsNotFound(err) { + config = &corev1.ConfigMap{} // Use default + } + ``` + - **Get before create** - Check existence before creating: + ```go + existing, err := c.lister.Get(name) + if apierrors.IsNotFound(err) { + // Resource doesn't exist, create it + return c.client.Create(ctx, required, metav1.CreateOptions{}) + } else if err != nil { + return fmt.Errorf("failed to check existing: %w", err) + } + ``` +- Return meaningful error messages + +### 6. Resource Application +- Use `resourceapply.Apply*()` functions from library-go +- Pass controller recorder for events +- Handle returned error properly + +### 7. OwnerReference Management +- Set owner references using `util.OwnerRefFrom(cr)` (from `pkg/console/subresource/util`) +- Be careful with multiple owner references (only one controller=true) +- Handle OwnerRef cleanup when replacing existing resources + +## Output Format + +For each issue found, report: +- **Location**: File:line or function name +- **Issue**: What pattern is missing or incorrect +- **Fix**: How to correct it +- **Severity**: Critical / Warning / Suggestion + +## Example Review Comments + +**Critical**: Missing ManagementState.Removed handling in Sync() function. Controller will not clean up resources when console is removed. + +**Warning**: Status conditions not flushed. Add `statusHandler.FlushAndReturn(err)` at end of Sync(). + +**Suggestion**: Consider grouping imports with standard comments for better readability. diff --git a/.claude/skills/e2e-test-review.md b/.claude/skills/e2e-test-review.md new file mode 100644 index 000000000..44925b5db --- /dev/null +++ b/.claude/skills/e2e-test-review.md @@ -0,0 +1,279 @@ +--- +name: e2e-test-review +description: Review e2e tests for console-operator framework patterns, best practices, and anti-patterns +tags: [e2e, testing, review] +--- + +# E2E Test Review Skill + +Review end-to-end tests in `test/e2e/` directory for correctness and best practices. + +## When to Use This Skill + +- PR modifies `test/e2e/*.go` files +- Reviewing e2e test implementations +- Detecting anti-patterns and missing best practices +- Verifying proper framework usage + +--- + +# Framework Patterns Reference + +These are the correct patterns to look for when reviewing e2e tests. + +## 1. Setup and Cleanup + +**Preferred pattern:** +```go +func TestFeature(t *testing.T) { + client, operatorConfig := framework.StandardSetup(t) + defer framework.StandardCleanup(t, client) + + // ... test logic +} +``` + +**Older acceptable pattern:** +```go +client := framework.MustNewClientset(t, nil) +operatorConfig, err := client.Operator.Consoles().Get( + context.TODO(), api.ConfigResourceName, metav1.GetOptions{}, +) +``` + +**Anti-pattern (flag in reviews):** +```go +// BAD - manual setup without framework +config, _ := clientcmd.BuildConfigFromFlags("", kubeconfig) +client, _ := kubernetes.NewForConfig(config) +``` + +## 2. Wait Patterns + +Use `wait.Poll` for eventual consistency (never `time.Sleep`): + +```go +err := wait.Poll(5*time.Second, framework.AsyncOperationTimeout, func() (bool, error) { + deployment, err := client.Apps.Deployments(ns).Get( + ctx, name, metav1.GetOptions{}, + ) + if err != nil { + return false, err + } + return deployment.Status.ReadyReplicas > 0, nil +}) +if err != nil { + t.Fatalf("deployment never became ready: %v", err) +} +``` + +**Poll interval guidance:** +- Use **5 seconds** for most checks (balances responsiveness with API load) +- Adjust based on what you're waiting for (faster for local state, slower for external resources) + +**Anti-pattern (flag in reviews):** +```go +// BAD - time.Sleep instead of wait.Poll +time.Sleep(30*time.Second) +deployment, _ := client.Apps.Deployments(ns).Get(ctx, name, metav1.GetOptions{}) +``` + +## 3. Retry Patterns + +Use `retry.RetryOnConflict` for updates: + +```go +err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + config, err := client.Operator.Consoles().Get( + ctx, api.ConfigResourceName, metav1.GetOptions{}, + ) + if err != nil { + return err + } + config.Spec.SomeField = "newValue" + _, err = client.Operator.Consoles().Update( + ctx, config, metav1.UpdateOptions{}, + ) + return err +}) +``` + +## 4. Context Usage + +Always use context with timeout: + +```go +ctx, cancel := context.WithTimeout(context.TODO(), framework.AsyncOperationTimeout) +defer cancel() +``` + +**Anti-pattern (flag in reviews):** +```go +// BAD - no timeout, context.Background instead of TODO +ctx := context.Background() +``` + +## 5. Cleanup and Error Handling + +### Cleanup with defer +```go +defer func() { + err := client.ConsolePluginV1.Delete( + ctx, pluginName, metav1.DeleteOptions{}, + ) + if err != nil && !apierrors.IsNotFound(err) { + t.Fatalf("failed to delete plugin: %v", err) + } +}() +``` + +**Anti-pattern (flag in reviews):** +```go +// BAD - missing cleanup, or ignoring errors +defer func() { + client.Delete(ctx, name, metav1.DeleteOptions{}) +}() +``` + +### Error handling +```go +// GOOD - helpful error message +if deployment.Status.ReadyReplicas == 0 { + t.Fatalf("deployment %s/%s has 0 ready replicas after %v", + ns, name, framework.AsyncOperationTimeout) +} + +// BAD - vague error +if deployment.Status.ReadyReplicas == 0 { + t.Fatal("not ready") +} +``` + +--- + +# Review Checklist + +When reviewing e2e tests, verify: + +- [ ] Uses `framework.StandardSetup(t)` for test setup (not `MustNewClientset`) +- [ ] Uses `framework.StandardCleanup(t, client)` with defer for cleanup +- [ ] Creates context with timeout (`context.WithTimeout`) +- [ ] Uses `retry.RetryOnConflict` for config updates +- [ ] Uses `wait.Poll` for async operations (not `time.Sleep`) + - 5-second intervals for most checks (balances responsiveness with API load) + - Interval adjusted based on what's being polled +- [ ] Uses framework helpers (`GetConsoleDeployment`, `GetConsoleConfigMap`, etc.) +- [ ] Has helpful error messages with context +- [ ] Handles `IsNotFound` errors in cleanup (`if err != nil && !apierrors.IsNotFound(err)`) +- [ ] Uses table-driven tests for multiple similar cases (when appropriate) + +## Anti-Patterns to Flag + +### Critical Issues + +1. **Missing cleanup** + - No `defer` for resource cleanup + - Not handling cleanup errors properly + +2. **Using `time.Sleep` instead of `wait.Poll`** + - Causes flaky tests and wastes time + +3. **Missing context timeouts** + - Tests can hang indefinitely + +4. **Ignoring errors with `_`** + - No error assertions in tests + +5. **Tests without assertions** + - Test runs but doesn't verify anything + +### Warnings + +1. **Manual client setup instead of framework.StandardSetup** + - Misses pristine state setup and cleanup + +2. **Vague error messages** + - `t.Fatal("failed")` instead of contextual errors + +3. **Not using framework helpers** + - Manual `client.Apps.Deployments().Get()` instead of `framework.GetConsoleDeployment()` + +4. **Missing table-driven structure** + - Testing multiple similar cases without using table-driven pattern + +### Suggestions + +1. **Consider grouping imports** + - Standard lib, 3rd party, kube, openshift, internal + +2. **Use framework constants** + - `framework.AsyncOperationTimeout` instead of hardcoded durations + +## Review Output Format + +For each issue found, report: +- **File:Line**: Location of the issue +- **Issue**: What pattern is wrong or missing +- **Fix**: How to correct it +- **Priority**: Critical / Warning / Suggestion + +### Example Review Comments + +**Critical**: `test/e2e/feature_test.go:45` - Missing cleanup for created ConsolePlugin resource. Add defer with Delete() and IsNotFound check. + +**Warning**: `test/e2e/feature_test.go:67` - Using `time.Sleep(30*time.Second)` instead of `wait.Poll`. Replace with `wait.Poll(5*time.Second, framework.AsyncOperationTimeout, ...)` for eventual consistency. + +**Suggestion**: `test/e2e/feature_test.go:23` - Consider using `framework.StandardSetup(t)` instead of manual client setup for automatic pristine state management. + +--- + +# Framework API Reference + +## Setup and Cleanup + +- `framework.StandardSetup(t)` - Returns `(*ClientSet, *operatorv1.Console)`. Sets up client and pristine operator config. +- `framework.StandardCleanup(t, client)` - Restores operator to pristine state after test. + +## Resource Helpers + +Prefer these helpers over manual client access: + +- `framework.GetConsoleDeployment(client)` - Returns `(*appv1.Deployment, error)` +- `framework.GetDownloadsDeployment(client)` - Returns `(*appv1.Deployment, error)` +- `framework.GetConsoleConfigMap(client)` - Returns `(*corev1.ConfigMap, error)` +- `framework.GetCustomLogoConfigMap(client, name)` - Returns `(*corev1.ConfigMap, error)` +- `framework.GetConsoleService(client)` - Returns `(*corev1.Service, error)` +- `framework.GetConsoleRoute(client)` - Returns `(*routev1.Route, error)` +- `framework.GetConsoleCLIDownloads(client, name)` - Returns `(*consolev1.ConsoleCLIDownload, error)` +- `framework.GetConsolePodDisruptionBudget(client, pdbName)` - Returns `(*policyv1.PodDisruptionBudget, error)` + +## ClientSet Interfaces + +When helper functions don't exist, use `framework.ClientSet` typed interfaces: + +**Core Kubernetes:** +- `client.Core` - CoreV1 resources (ConfigMaps, Services, Secrets) +- `client.Apps` - AppsV1 resources (Deployments, DaemonSets, StatefulSets) +- `client.PodDisruptionBudget` - PodDisruptionBudgets + +**OpenShift Routing:** +- `client.Routes` - OpenShift Routes + +**Console Operator:** +- `client.Operator` - Console operator configs (operatorv1.Console) + +**Console Config Resources:** +- `client.Console` - Cluster console configs (configv1.Console) +- `client.ConsolePluginV1` - Console plugins +- `client.ConsoleCliDownloads` - Console CLI downloads +- `client.ConsoleLink` - Console links +- `client.ConsoleNotification` - Console notifications +- `client.ConsoleExternalLogLink` - External log links +- `client.ConsoleYAMLSample` - YAML samples + +**Cluster Config:** +- `client.ClusterOperator` - ClusterOperator status +- `client.Infrastructure` - Cluster infrastructure config +- `client.Proxy` - Cluster proxy config +- `client.Ingress` - Cluster ingress config +- `client.FeatureGate` - Feature gates diff --git a/.claude/skills/go-quality-review.md b/.claude/skills/go-quality-review.md new file mode 100644 index 000000000..819f11789 --- /dev/null +++ b/.claude/skills/go-quality-review.md @@ -0,0 +1,318 @@ +--- +name: go-quality-review +description: Review Go code for quality, deprecated APIs, and common anti-patterns +tags: [review, go, quality, deprecated] +--- + +# Go Code Quality Review Skill + +Review Go code for quality issues, deprecated APIs, and common problems. + +## Deprecated API Detection + +### Check for Deprecated Imports and Functions + +**Deprecated (Do NOT use):** +```go +// DEPRECATED: Use os.ReadFile +ioutil.ReadFile(filename) +ioutil.WriteFile(filename, data, perm) +ioutil.ReadAll(reader) + +// DEPRECATED: Use DialContext +Dial: func(network, addr string) (net.Conn, error) { + return net.Dial(network, addr) +} + +// DEPRECATED: Various k8s.io/utils functions +// Check specific package documentation +``` + +**Modern alternatives:** +```go +// Use os package (Go 1.16+) +os.ReadFile(filename) +os.WriteFile(filename, data, perm) +io.ReadAll(reader) + +// Use DialContext +DialContext: func(ctx context.Context, network, addr string) (net.Conn, error) { + return (&net.Dialer{}).DialContext(ctx, network, addr) +} +``` + +## Error Handling + +### Error Wrapping +**Good:** +```go +if err != nil { + return fmt.Errorf("failed to get config: %w", err) +} +``` + +**Bad:** +```go +if err != nil { + return fmt.Errorf("failed to get config: %v", err) // Lost error chain +} +``` + +### Error Context +Add context to errors: +```go +// Good - explains what was happening +return fmt.Errorf("failed to sync deployment %s in namespace %s: %w", + name, namespace, err) + +// Bad - no context +return err +``` + +### Checking Specific Errors +```go +// Good - specific error check +if apierrors.IsNotFound(err) { + // Handle not found +} + +// Bad - string matching +if strings.Contains(err.Error(), "not found") { + // Fragile +} +``` + +## Resource Management + +### Context Propagation +**Good:** +```go +func (c *Controller) Sync(ctx context.Context, ...) error { + deployment, err := c.client.Get(ctx, name, metav1.GetOptions{}) + // ... +} +``` + +**Bad:** +```go +func (c *Controller) Sync(ctx context.Context, ...) error { + deployment, err := c.client.Get(context.Background(), name, metav1.GetOptions{}) + // Ignores parent context cancellation +} +``` + +### Defer Usage +```go +// Good - cleanup on all paths +func doWork(ctx context.Context) error { + resource, err := acquire() + if err != nil { + return err + } + defer release(resource) + + return performWork(resource) +} + +// Bad - leaks on early return +func doWork(ctx context.Context) error { + resource, err := acquire() + if err != nil { + return err // LEAK! + } + + if someCondition { + return errors.New("failed") // LEAK! + } + + release(resource) + return nil +} +``` + +## Code Smells + +### God Functions +Flag functions longer than ~100 lines or with too many responsibilities: +```go +// BAD - doing too much +func (c *Controller) Sync(...) error { + // 50 lines of config validation + // 100 lines of deployment logic + // 75 lines of service setup + // 80 lines of route configuration + // = 305 lines, should be split +} + +// GOOD - extracted +func (c *Controller) Sync(...) error { + if err := c.validateConfig(); err != nil { + return err + } + if err := c.syncDeployment(); err != nil { + return err + } + if err := c.syncService(); err != nil { + return err + } + return c.syncRoute() +} +``` + +### Magic Values +```go +// Bad - magic numbers/strings +if replicas == 2 { + // Why 2? +} +timeout := 30 * time.Second // Why 30? +if mode == "special" { + // What makes it special? +} + +// Good - named constants +const ( + DefaultReplicaCount = 2 // HA requirement + DefaultTimeout = 30 * time.Second + SpecialMode = "special" // Tech preview mode +) +``` + +### Deep Nesting +```go +// Bad - deeply nested +if condition1 { + if condition2 { + if condition3 { + if condition4 { + // business logic buried here + } + } + } +} + +// Good - early returns +if !condition1 { + return nil +} +if !condition2 { + return nil +} +if !condition3 { + return nil +} +if !condition4 { + return nil +} +// business logic at top level +``` + +## Performance + +### String Building +```go +// Bad - inefficient string concatenation +var result string +for _, item := range items { + result += item + "\n" +} + +// Good - strings.Builder +var builder strings.Builder +for _, item := range items { + builder.WriteString(item) + builder.WriteString("\n") +} +result := builder.String() +``` + +### Unnecessary Allocations +```go +// Bad - allocates on every call +func (c *Controller) getNamespace() string { + return string([]byte("openshift-console")) // Unnecessary byte slice allocation +} + +// Good - use constant +const namespace = "openshift-console" +``` + +## Concurrency + +### Missing Mutex Protection +```go +// Bad - race condition +type Cache struct { + data map[string]string +} +func (c *Cache) Set(k, v string) { + c.data[k] = v // RACE if called from multiple goroutines +} + +// Good - protected +type Cache struct { + mu sync.RWMutex + data map[string]string +} +func (c *Cache) Set(k, v string) { + c.mu.Lock() + defer c.mu.Unlock() + c.data[k] = v +} +``` + +## Testing + +### Missing Error Checks +```go +// Bad - ignoring errors in tests +result, _ := doSomething() +if result != expected { + t.Error("mismatch") +} + +// Good - checking errors +result, err := doSomething() +if err != nil { + t.Fatalf("unexpected error: %v", err) +} +if result != expected { + t.Errorf("expected %v, got %v", expected, result) +} +``` + +## Documentation + +### Exported Functions +```go +// Good - documented export +// NewController creates a new console controller. +// It requires a valid operatorClient and informer factory. +func NewController(...) factory.Controller { + // ... +} + +// Bad - no doc +func NewController(...) factory.Controller { + // ... +} +``` + +## Output Format + +For each issue: +- **File:Line**: Location +- **Issue**: What's wrong +- **Category**: Deprecated / Error Handling / Code Smell / Performance / etc. +- **Fix**: How to improve +- **Priority**: High / Medium / Low + +## Example Review Comments + +**High Priority**: pkg/console/operator/sync.go:45 - Using deprecated ioutil.ReadFile. Replace with os.ReadFile. + +**Medium Priority**: pkg/console/controllers/route/controller.go:123 - Deep nesting (5 levels). Consider early returns. + +**Low Priority**: Consider adding godoc comment for exported function NewRouteController. + +**Info**: Good use of context propagation throughout sync handler. diff --git a/.claude/skills/manifest-review.md b/.claude/skills/manifest-review.md new file mode 100644 index 000000000..68f686381 --- /dev/null +++ b/.claude/skills/manifest-review.md @@ -0,0 +1,102 @@ +--- +name: manifest-review +description: Review RBAC manifests and CVO manifest files for cluster profile annotations and correctness +tags: [review, manifest, rbac, cvo] +--- + +# Manifest Review Skill + +Review Kubernetes manifest files in `manifests/`, `bindata/assets/`, `quickstarts/`, and `examples/` directories. + +## What to Check + +### 1. Cluster Profile Annotations +All CVO manifests MUST include appropriate cluster profile annotations: + +```yaml +annotations: + include.release.openshift.io/hypershift: "true" + include.release.openshift.io/ibm-cloud-managed: "true" + include.release.openshift.io/self-managed-high-availability: "true" + include.release.openshift.io/single-node-developer: "true" + capability.openshift.io/name: Console +``` + +**Common profiles:** +- `hypershift` - HyperShift hosted control planes +- `ibm-cloud-managed` - IBM Cloud managed OpenShift +- `self-managed-high-availability` - Standard self-managed HA clusters +- `single-node-developer` - Single-node OpenShift (SNO) + +### 2. Capability Annotations +Console resources should include: +```yaml +capability.openshift.io/name: Console +``` + +### 3. RBAC Rules +Check RBAC manifests for: +- **Principle of least privilege**: Only grant necessary permissions +- **Resource specificity**: Avoid wildcards unless truly needed +- **Verb appropriateness**: Match verbs to actual needs (get/list/watch vs create/update/delete) +- **apiGroups correctness**: Use proper API groups ("" for core, specific groups for CRDs) + +### 4. Namespace Consistency +- Resources in `openshift-console` namespace for console workload +- Resources in `openshift-console-operator` namespace for operator +- Cross-namespace references are explicit and intentional + +### 5. YAML Formatting +- Proper indentation (2 spaces) +- Consistent ordering of fields +- Include `---` separator between multiple resources in one file + +### 6. Service Account References +When binding roles to service accounts, verify which SA is appropriate: + +**Operator SA (cross-namespace bindings):** +```yaml +subjects: + - kind: ServiceAccount + name: console-operator + namespace: openshift-console-operator +``` + +**Console workload SA (same-namespace bindings):** +```yaml +subjects: + - kind: ServiceAccount + name: console + namespace: openshift-console +``` + +> Verify which SA is appropriate based on what the role grants access to: +> use `console-operator` when the operator needs cross-namespace permissions, +> and `console` when the binding is scoped to the console workload in `openshift-console`. + +### 7. Profile-Specific Patches +Check `profile-patches/` for overrides that should only apply to specific deployments. + +## Red Flags + +- Missing cluster profile annotations on new manifests +- RBAC with `*` wildcards without clear justification +- Cross-namespace permissions without clear need +- Missing capability annotations +- Hardcoded values that should be configurable + +## Output Format + +For each issue: +- **File**: manifest filename +- **Issue**: What's wrong +- **Impact**: Which clusters/profiles are affected +- **Fix**: Recommended correction + +## Example Review Comments + +**Critical**: manifests/new-role.yaml missing cluster profile annotations. This resource won't be deployed to any clusters. + +**Warning**: RBAC grants `*` verbs on configmaps. Consider restricting to specific verbs needed. + +**Info**: Consider adding hypershift annotation if this resource is needed for hosted control planes. diff --git a/.claude/skills/sync-handler-review.md b/.claude/skills/sync-handler-review.md new file mode 100644 index 000000000..824ba3a0a --- /dev/null +++ b/.claude/skills/sync-handler-review.md @@ -0,0 +1,151 @@ +--- +name: sync-handler-review +description: Review operator sync handler logic for incremental reconciliation patterns +tags: [review, sync, reconciliation, operator] +--- + +# Sync Handler Review Skill + +Review sync handler implementations (especially `sync_v400.go` and controller Sync methods). + +## What to Check + +### 1. Incremental Sync Pattern +The operator uses an incremental sync pattern where each loop picks up where the previous left off: + +**Good Pattern:** +```go +func (c *OperatorController) Sync(ctx context.Context, ...) error { + // 1. Check prerequisite resources + requiredConfig, err := c.getRequiredConfig() + if err != nil { + return err // Will retry + } + + // 2. Sync first dependency + if err := c.syncConfigMaps(ctx, requiredConfig); err != nil { + return err // Stop here, retry next loop + } + + // 3. Sync second dependency (only if step 2 succeeded) + if err := c.syncSecrets(ctx); err != nil { + return err + } + + // 4. Sync final resource + return c.syncDeployment(ctx, requiredConfig) +} +``` + +**Bad Pattern:** +```go +func (c *OperatorController) Sync(ctx context.Context, ...) error { + // DON'T collect all errors and continue + var errs []error + errs = append(errs, c.syncConfigMaps(ctx)) + errs = append(errs, c.syncSecrets(ctx)) + errs = append(errs, c.syncDeployment(ctx)) + return errors.Join(errs...) +} +``` + +### 2. Dependency Ordering +Resources should be synced in dependency order: +1. ConfigMaps and Secrets (prerequisites) +2. Service Accounts +3. RBAC (Roles, RoleBindings) +4. Services +5. Deployments (depend on above) +6. Routes (depend on Services) + +### 3. Status Updates +Status should reflect actual reconciliation state: +```go +statusHandler := status.NewStatusHandler(c.operatorClient) + +// Add conditions as operations complete +if err := c.syncResource(ctx); err != nil { + statusHandler.AddConditions( + status.HandleProgressingOrDegraded("ResourceSync", "FailedApply", err) + ) + return statusHandler.FlushAndReturn(err) +} + +// Mark available when fully synced +statusHandler.AddCondition( + status.HandleAvailable("DeploymentAvailable", "", nil) +) +return statusHandler.FlushAndReturn(nil) +``` + +### 4. Early Returns +Return early on errors to maintain incremental behavior: +```go +// GOOD - returns immediately +if err := c.doSomething(); err != nil { + return err +} + +// BAD - continues after error +if err := c.doSomething(); err != nil { + klog.Errorf("failed: %v", err) +} +``` + +### 5. Resource Creation vs Update +Use `resourceapply.Apply*()` functions - they handle both create and update: +```go +_, _, err = resourceapply.ApplyConfigMap( + ctx, + c.configMapClient, + recorder, + requiredConfigMap, +) +``` + +### 6. Deleted Resource Detection +When resources are removed from config, ensure they're deleted from cluster: +```go +if !shouldExist { + err := c.client.Delete(ctx, name, metav1.DeleteOptions{}) + if apierrors.IsNotFound(err) { + return nil // Already gone, success + } + return err +} +``` + +### 7. Feature Gate Handling +Check feature gates before syncing gated resources: +```go +if featureGates.Enabled(features.ExternalOIDC) { + if err := c.syncOIDCResources(ctx); err != nil { + return err + } +} +``` + +## Anti-Patterns + +- Syncing all resources regardless of errors +- Continuing after failure instead of returning +- Not respecting ManagementState +- Missing cleanup logic for Removed state +- Mutating live objects instead of building desired state +- Lack of status condition updates + +## Output Format + +For each issue: +- **Location**: Function or line range +- **Pattern**: Which pattern is violated +- **Risk**: What could go wrong +- **Fix**: How to correct it + +## Example Review Comments + +**Critical**: Sync continues after configmap error. This violates incremental sync pattern and could deploy with incomplete config. + +**Warning**: No cleanup logic for ManagementState.Removed. Resources will leak when console is removed. + +**Suggestion**: Consider extracting this sync logic into a separate controller for better separation of concerns. diff --git a/.claude/skills/unit-test-review.md b/.claude/skills/unit-test-review.md new file mode 100644 index 000000000..3c0f5ec35 --- /dev/null +++ b/.claude/skills/unit-test-review.md @@ -0,0 +1,301 @@ +--- +name: unit-test-review +description: Review Go unit tests for best practices, table-driven patterns, and proper assertions +tags: [review, testing, unit, go] +--- + +# Unit Test Review Skill + +Review unit tests in `pkg/**/*_test.go` files for correctness and best practices. + +## What to Check + +### 1. Table-Driven Test Pattern +Most unit tests should use table-driven pattern for clarity and completeness: +```go +func TestFeature(t *testing.T) { + tests := []struct { + name string + input InputType + expected OutputType + wantErr bool + }{ + { + name: "happy path", + input: validInput, + expected: expectedOutput, + wantErr: false, + }, + { + name: "error case", + input: invalidInput, + expected: OutputType{}, + wantErr: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := FunctionUnderTest(tt.input) + + if (err != nil) != tt.wantErr { + t.Errorf("expected error: %v, got: %v", tt.wantErr, err) + } + + if diff := deep.Equal(result, tt.expected); diff != nil { + t.Error(diff) + } + }) + } +} +``` + +### 2. Test Naming +**Good test names:** +- `TestGetNodeComputeEnvironments` - describes what function is tested +- `TestNewRouteConfig` - clear and descriptive +- Test case names: `"Custom hostname and TLS secret set"` - explains scenario + +**Bad test names:** +- `TestFunc1` - not descriptive +- `TestIt` - what is "it"? +- Test case names: `"test1"` - doesn't explain what's being tested + +### 3. Deep Equality Checks +Use `github.com/go-test/deep` for struct comparisons: +```go +// Good - shows exact differences +if diff := deep.Equal(actual, expected); diff != nil { + t.Error(diff) +} + +// Bad - unhelpful error message +if actual != expected { + t.Error("mismatch") +} + +// Bad - manual field-by-field comparison (fragile) +if actual.Field1 != expected.Field1 || actual.Field2 != expected.Field2 { + t.Error("fields don't match") +} +``` + +### 4. Test Coverage +**Test both success and failure paths:** +```go +tests := []struct { + name string + input string + wantErr bool +}{ + {name: "valid input", input: "valid", wantErr: false}, + {name: "empty input", input: "", wantErr: true}, + {name: "invalid format", input: "bad", wantErr: true}, +} +``` + +**Edge cases to consider:** +- Empty inputs (nil, "", empty slices/maps) +- Boundary values (0, -1, max int) +- Missing labels/fields +- Duplicate values +- Large inputs + +### 5. Test Structure (Arrange-Act-Assert) +```go +t.Run("test scenario", func(t *testing.T) { + // Arrange - setup test data + config := &operatorv1.Console{ + Spec: operatorv1.ConsoleSpec{ + Route: operatorv1.ConsoleConfigRoute{ + Hostname: "custom.example.com", + }, + }, + } + + // Act - call the function + result := ProcessConfig(config) + + // Assert - verify results + if diff := deep.Equal(result, expected); diff != nil { + t.Error(diff) + } +}) +``` + +### 6. Error Handling +**Check error presence:** +```go +// Good - checks if error occurred when expected +if (err != nil) != tt.wantErr { + t.Errorf("wantErr %v, got error: %v", tt.wantErr, err) +} + +// Bad - ignores error +result, _ := FunctionUnderTest(input) +``` + +**Check error messages (when specific):** +```go +if err != nil && !strings.Contains(err.Error(), "expected substring") { + t.Errorf("unexpected error message: %v", err) +} +``` + +### 7. Mocking and Test Isolation +**Prefer interfaces for testability:** +```go +// Good - uses interface, easy to mock +type ConfigGetter interface { + GetConfig() (*Config, error) +} + +func ProcessData(getter ConfigGetter) error { + config, err := getter.GetConfig() + // ... +} + +// Bad - hard-coded dependency +func ProcessData() error { + config := getConfigFromKubernetes() // can't test without cluster + // ... +} +``` + +**Keep tests isolated:** +- Tests should not depend on execution order +- Tests should not share mutable state +- Each test case should be independent + +### 8. Helper Functions +Extract common setup into helpers: +```go +func testConsole() *operatorv1.Console { + return &operatorv1.Console{ + Spec: operatorv1.ConsoleSpec{ + // common test config + }, + } +} + +func TestMultipleFunctions(t *testing.T) { + console := testConsole() + // modify as needed for specific test +} +``` + +### 9. Assertion Quality +**Good assertions:** +```go +// Specific error message with context +if result != expected { + t.Errorf("expected %d nodes, got %d", expected, result) +} + +// Shows what differs +if diff := deep.Equal(actual, expected); diff != nil { + t.Errorf("config mismatch: %v", diff) +} +``` + +**Bad assertions:** +```go +// Vague - what failed? +if result != expected { + t.Error("failed") +} + +// Silent failure +if result != expected { + // no error reported +} +``` + +### 10. Test Data Management +**Inline simple data:** +```go +tests := []struct { + name string + input int +}{ + {name: "zero", input: 0}, + {name: "one", input: 1}, +} +``` + +**Extract complex fixtures:** +```go +// testdata/valid_config.yaml +// or helper functions for complex objects +func validConsoleConfig() *operatorv1.Console { + return &operatorv1.Console{ + // complex config + } +} +``` + +## Red Flags + +- No table-driven tests for functions with multiple scenarios +- Tests without subtests (can't see which case failed) +- Tests that depend on execution order +- Global mutable state between tests +- Hardcoded sleeps (use mocks/fakes instead) +- Tests without assertions (just calling the function) +- Ignoring errors with `_` +- Testing implementation details instead of behavior +- Overly complex test setup (refactor the code, not the test) + +## Common Patterns in This Codebase + +### Using go-test/deep +```go +import "github.com/go-test/deep" + +if diff := deep.Equal(actual, expected); diff != nil { + t.Error(diff) +} +``` + +### Testing Functions That Return Errors +```go +result, err := FunctionUnderTest(input) + +if (err != nil) != tt.wantErr { + t.Errorf("expected error: %v, got: %v", tt.wantErr, err) + return +} + +if !tt.wantErr && diff := deep.Equal(result, tt.expected); diff != nil { + t.Error(diff) +} +``` + +### Testing with Kubernetes Objects +```go +node := &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-node", + Labels: map[string]string{ + "key": "value", + }, + }, +} +``` + +## Output Format + +For each issue: +- **Test**: Test function name +- **Issue**: What's wrong +- **Impact**: Why it matters +- **Fix**: How to improve + +## Example Review Comments + +**Critical**: TestProcessConfig has no assertions - test always passes even if function is broken. + +**Warning**: Using `==` to compare structs instead of `deep.Equal` - test won't show what differs. + +**Suggestion**: Consider adding test case for empty input to TestGetNodes - edge case not covered. + +**Info**: Good use of table-driven tests with clear scenario names. diff --git a/.coderabbit.yaml b/.coderabbit.yaml index 9dae7c411..5e6963c38 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,5 +1,18 @@ # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json +# CodeRabbit configuration for console-operator +# This config integrates Claude Code skills for operator-specific reviews. +# Skills are triggered based on CODE PATTERNS (function signatures, types, YAML kinds) +# rather than file paths alone. +# +# Available skills (defined in .claude/skills/): +# /controller-review - When code has Controller types, factory.New() patterns +# /sync-handler-review - When code has sequential sync logic with early returns +# /manifest-review - When YAML has RBAC kinds or missing annotations +# /unit-test-review - When pkg/**/*_test.go has table-driven tests, go-test/deep +# /e2e-test-review - When test/e2e/ tests use framework, wait.Poll, retry patterns +# /go-quality-review - Always applied; checks deprecated APIs, code smells + language: en-US inheritance: true @@ -14,28 +27,99 @@ reviews: collapse_walkthrough: false review_details: true path_filters: - - "!vendor/**" - - "!**/*_test.go" + - '!vendor/**' path_instructions: - - path: "pkg/**/*.go" + - path: '**/*.go' instructions: | Review Go code following OpenShift operator patterns. See CONVENTIONS.md for coding standards and patterns. - Controllers should use the library-go factory pattern. - Status conditions should use status.Handle* functions. - - path: "bindata/assets/**/*.yaml" - instructions: | - Review YAML assets for Kubernetes resource correctness. - Ensure proper annotations for cluster profiles. - - path: "manifests/**/*.yaml" + + Refer to the following skills based on CODE PATTERNS, not just file paths: + + **Refer to /controller-review when code contains:** + - Controller struct types (e.g., `type *Controller struct`) + - `func New*Controller(` factory functions + - `factory.New().WithFilteredEventsInformers(` pattern + - `.ToController(` method calls + - `Sync(ctx context.Context, controllerContext factory.SyncContext)` methods + - `operatorConfig.Spec.ManagementState` checks + - `status.NewStatusHandler` or `status.Handle*` functions + + **Refer to /sync-handler-review when code contains:** + - Main operator sync functions (e.g., `sync_v400.go` content) + - Sequential resource syncing with early returns + - Incremental reconciliation loops + - Multiple `resourceapply.Apply*()` calls in sequence + - Dependency ordering of ConfigMaps → Secrets → Service Accounts → RBAC → Services → Deployments → Routes + - Feature gate conditional logic + + **Refer to /go-quality-review for all Go code to check:** + - Deprecated imports: `ioutil.ReadFile`, `ioutil.WriteFile`, `ioutil.ReadAll` + - Deprecated patterns: `Dial` without `DialContext` + - Error handling: missing `%w` in fmt.Errorf + - Code smells: deep nesting (4+ levels), functions >100 lines + - Magic values: unexplained numbers/strings + - Context propagation: `context.Background()` instead of passed ctx + - Missing godoc on exported functions + + - path: '**/*_test.go' instructions: | - Review CVO manifests carefully. - These are deployed to clusters and changes have wide impact. - - path: "quickstarts/**/*.yaml" + Review test code for quality and patterns. + + **Refer to /unit-test-review when test is in pkg/**/*_test.go:** + - Table-driven test structure with test cases + - Use of `go-test/deep` for struct comparisons + - Test naming conventions (TestFunctionName) + - Error handling with `wantErr` pattern + - Edge case coverage (nil, empty, boundary values) + - Proper assertions with helpful error messages + - Test isolation (no shared mutable state) + + **Refer to /e2e-test-review when test contains:** + - `framework.MustNewClientset(t, nil)` or similar e2e framework usage + - `wait.Poll` or `wait.PollImmediate` patterns + - `retry.RetryOnConflict` for updates + - Cleanup via `defer` functions + - Console/operator CR manipulations + - Test assertions on cluster state + + **Suggest to use /e2e-test-review when:** + - PR adds new feature requiring e2e coverage + - Test file is empty or skeleton + - Comments indicate "TODO: add test" + + **Review for common issues:** + - Missing cleanup (defer statements) + - Using `time.Sleep` instead of `wait.Poll` + - Missing context timeouts + - Vague error messages in assertions + - Tests without table-driven structure when testing multiple cases + - Ignoring errors with `_` + - Tests without assertions + + - path: '**/*.yaml' instructions: | - Review quickstart YAML definitions. - Ensure cluster profile annotations are present. - See quickstarts/README.md for guidelines. + Review YAML manifests based on content and kind. + + **Refer to /manifest-review when YAML contains:** + - `kind: Role` or `kind: ClusterRole` (RBAC review) + - `kind: RoleBinding` or `kind: ClusterRoleBinding` + - `annotations:` section (check for cluster profiles) + - `verbs: ["*"]` or wildcard permissions + - `apiGroups: ["*"]` or overly broad permissions + - ServiceAccount references in subjects + + **Check for required annotations in manifests/:** + - `include.release.openshift.io/hypershift` + - `include.release.openshift.io/ibm-cloud-managed` + - `include.release.openshift.io/self-managed-high-availability` + - `include.release.openshift.io/single-node-developer` + - `capability.openshift.io/name: Console` + + **For quickstarts/, additionally check:** + - QuickStart spec structure + - Task descriptions and prerequisites + - See quickstarts/README.md for guidelines chat: auto_reply: true @@ -46,13 +130,14 @@ knowledge_base: code_guidelines: enabled: true filePatterns: - - "AGENTS.md" - - "CLAUDE.md" - - "ARCHITECTURE.md" - - "CONVENTIONS.md" - - "TESTING.md" + - 'AGENTS.md' + - 'CLAUDE.md' + - 'ARCHITECTURE.md' + - 'CONVENTIONS.md' + - 'TESTING.md' + - '.claude/skills/*.md' linked_repositories: - - repository: "openshift/console" + - repository: 'openshift/console' instructions: >- Console is the downstream UI that consumes this operator's configuration. pkg/console/subresource/consoleserver/types.go is the canonical source diff --git a/.github/CODERABBIT_SETUP.md b/.github/CODERABBIT_SETUP.md new file mode 100644 index 000000000..9e8c610ce --- /dev/null +++ b/.github/CODERABBIT_SETUP.md @@ -0,0 +1,130 @@ +# CodeRabbit and Claude Code Integration Setup + +This document describes the AI tooling integration for the console-operator repository. + +## Overview + +The console-operator uses two complementary AI tools: +- **Claude Code**: Interactive development assistant with custom operator skills +- **CodeRabbit**: Automated PR review bot that references Claude skills + +## Claude Code Skills + +Custom skills are defined in `.claude/skills/`: + +| Skill | Purpose | Use When | +|-------|---------|----------| +| `controller-review` | Review controller patterns | Reviewing controller implementations | +| `sync-handler-review` | Review sync/reconciliation logic | Reviewing operator sync handlers | +| `manifest-review` | Review RBAC/CVO manifests | Reviewing manifest changes | +| `unit-test-review` | Review unit tests | Reviewing pkg/**/*_test.go files | +| `e2e-test-review` | Review e2e tests | Reviewing or creating test/e2e/**/*_test.go files | +| `go-quality-review` | Review code quality | Checking for deprecated APIs, code smells | + +### Using Skills + +In Claude Code CLI or IDE extensions: +```bash +# Review existing code +/controller-review +/e2e-test-review + +# When generating new e2e tests, reference patterns from the review skill: +# "Reference /e2e-test-review skill for framework patterns" +``` + +In PR reviews, CodeRabbit will reference these automatically based on code patterns, not just file paths. + +## CodeRabbit Configuration + +Configuration is in `.coderabbit.yaml`. It: +- References project documentation (ARCHITECTURE.md, CONVENTIONS.md, etc.) +- Applies review guidelines from skills based on **code patterns** (function signatures, types, YAML kinds) not just file paths +- Integrates with openshift/console repository context +- Provides operator-specific review guidelines + +> **Note**: CodeRabbit embeds skill content as reviewer instructions during PR review. +> Skills are also usable interactively via Claude Code (e.g., `/controller-review`). + +### Pattern-Based Review Guidelines + +Review guidelines from skills are applied when specific code patterns are detected: + +**controller-review** applies when code contains: +- `type *Controller struct` definitions +- `factory.New().WithFilteredEventsInformers()` pattern +- `Sync(ctx context.Context, controllerContext factory.SyncContext)` methods +- `status.NewStatusHandler` usage + +**sync-handler-review** applies when code contains: +- Sequential `resourceapply.Apply*()` calls with early returns +- Feature gate conditional logic +- Incremental reconciliation patterns + +**manifest-review** applies when YAML contains: +- `kind: Role` or `kind: ClusterRole` +- Missing cluster profile annotations in `manifests/` +- Wildcard permissions (`verbs: ["*"]`) + +**e2e-test-review** applies when tests contain: +- `framework.StandardSetup(t)` or `framework.MustNewClientset(t, nil)` +- `wait.Poll` or `retry.RetryOnConflict` +- Console/operator CR manipulations + +**go-quality-review** applies to all Go code to check: +- Deprecated APIs (`ioutil.*`, `Dial` without `DialContext`) +- Error handling patterns (missing `%w` in `fmt.Errorf`) +- Code smells (deep nesting, god functions, magic values) + +This approach is more reliable than path-based matching, as it recognizes what the code actually does. + +## CI/CD Integration + +### Skip E2E Tests for Tooling Changes + +Changes to AI tooling configuration should not trigger e2e test runs. + +**This repository (console-operator):** +- OWNERS file documents the intent (see comments) +- `.coderabbit.yaml` contains the configuration +- `.claude/skills/` contains Claude skills + +**openshift/release repository:** + +To skip e2e tests for tooling changes, add to the ci-operator configuration for console-operator: + +```yaml +skip_if_only_changed: "^(\\.coderabbit\\.yaml|\\.claude/|.*\\.md|OWNERS)$" +``` + +Or in Prow configuration: +```yaml +- name: pull-ci-openshift-console-operator-main-e2e + skip_if_only_changed: "^(\\.coderabbit\\.yaml|\\.claude/|.*\\.md|OWNERS)$" +``` + +**Action Required:** Update openshift/release configuration to implement this skip pattern. + +## Updating Skills + +To add or modify Claude skills: + +1. Edit or create skill files in `.claude/skills/` +2. Update `.coderabbit.yaml` path_instructions to reference new skills +3. Test the skill locally with Claude Code: `/skill-name` +4. Commit changes (will not trigger e2e tests once openshift/release is updated) + +## Syncing with openshift/console + +The console repository has similar AI tooling. When adding patterns here, consider: +- Are there equivalent patterns for TypeScript/React code? +- Should frontend-specific skills be added to console repo? +- Do both repos need to reference the same conventions? + +Keep tooling configuration aligned between repos where patterns overlap. + +## Reference + +- [Claude Code Documentation](https://docs.anthropic.com/claude/docs) +- [CodeRabbit Documentation](https://coderabbit.ai/docs) +- [OpenShift CI/CD Docs](https://docs.ci.openshift.org/)