diff --git a/.claude/skills/controller-review.md b/.claude/skills/controller-review.md new file mode 100644 index 000000000..1c2da6692 --- /dev/null +++ b/.claude/skills/controller-review.md @@ -0,0 +1,93 @@ +--- +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 +- 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 `subresourceutil.OwnerRefFrom(cr)` +- 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-create.md b/.claude/skills/e2e-test-create.md new file mode 100644 index 000000000..acf81c2a6 --- /dev/null +++ b/.claude/skills/e2e-test-create.md @@ -0,0 +1,320 @@ +--- +name: e2e-test-create +description: Scaffold new e2e tests following console-operator patterns and best practices +tags: [create, testing, e2e, scaffold] +--- + +# E2E Test Creation Skill + +Create new end-to-end tests in `test/e2e/` directory following established patterns. + +## Test Template + +```go +package e2e + +import ( + "context" + "testing" + "time" + + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/console-operator/pkg/api" + "github.com/openshift/console-operator/test/e2e/framework" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" +) + +func TestYourFeature(t *testing.T) { + // Setup client and get pristine operator config + client, operatorConfig := framework.StandardSetup(t) + defer framework.StandardCleanup(t, client) + + ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Minute) + defer cancel() + + // Check management state + if operatorConfig.Spec.ManagementState != operatorv1.Managed { + t.Skip("Skipping test - operator not in Managed state") + } + + // Perform operations + err := updateOperatorConfig(ctx, client, func(config *operatorv1.Console) { + // Modify config + }) + if err != nil { + t.Fatalf("failed to update config: %v", err) + } + + // Wait for expected state + err = waitForCondition(ctx, t, client) + if err != nil { + t.Fatalf("expected condition not met: %v", err) + } + + // Verify result using framework helpers + deployment, err := framework.GetConsoleDeployment(client) + if err != nil { + t.Fatalf("failed to get deployment: %v", err) + } + if deployment.Status.ReadyReplicas == 0 { + t.Fatal("expected ready replicas > 0") + } +} + +// Helper functions +func updateOperatorConfig( + ctx context.Context, + client *framework.ClientSet, + modify func(*operatorv1.Console), +) error { + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + config, err := client.Operator.Consoles().Get( + ctx, api.ConfigResourceName, metav1.GetOptions{}, + ) + if err != nil { + return err + } + modify(config) + _, err = client.Operator.Consoles().Update( + ctx, config, metav1.UpdateOptions{}, + ) + return err + }) +} + +func waitForCondition( + ctx context.Context, + t *testing.T, + client *framework.ClientSet, +) error { + return wait.Poll(1*time.Second, 2*time.Minute, func() (bool, error) { + // Use framework helper instead of manual client access + deployment, err := framework.GetConsoleDeployment(client) + if err != nil { + return false, err + } + return deployment.Status.ReadyReplicas > 0, nil + }) +} +``` + +## Table-Driven Test Template + +```go +func TestMultipleScenarios(t *testing.T) { + client, _ := framework.StandardSetup(t) + defer framework.StandardCleanup(t, client) + + testCases := []struct { + name string + setup func(*operatorv1.Console) + expectedValue string + expectError bool + }{ + { + name: "scenario 1", + setup: func(config *operatorv1.Console) { + config.Spec.SomeField = "value1" + }, + expectedValue: "result1", + expectError: false, + }, + { + name: "scenario 2", + setup: func(config *operatorv1.Console) { + config.Spec.SomeField = "value2" + }, + expectedValue: "result2", + expectError: false, + }, + } + + ctx, cancel := context.WithTimeout(context.TODO(), 10*time.Minute) + defer cancel() + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Setup + err := updateOperatorConfig(ctx, client, tc.setup) + if err != nil { + t.Fatalf("setup failed: %v", err) + } + + // Execute test + result, err := performOperation(ctx, client) + + // Verify + if tc.expectError && err == nil { + t.Error("expected error but got none") + } + if !tc.expectError && err != nil { + t.Errorf("unexpected error: %v", err) + } + if result != tc.expectedValue { + t.Errorf("expected %v, got %v", tc.expectedValue, result) + } + }) + } +} +``` + +## Plugin Test Pattern + +```go +func TestConsolePlugin(t *testing.T) { + client, _ := framework.StandardSetup(t) + defer framework.StandardCleanup(t, client) + + ctx, cancel := context.WithTimeout(context.TODO(), 5*time.Minute) + defer cancel() + + pluginName := "test-plugin" + + // Create plugin + plugin := &consolev1.ConsolePlugin{ + ObjectMeta: metav1.ObjectMeta{ + Name: pluginName, + }, + Spec: consolev1.ConsolePluginSpec{ + DisplayName: "Test Plugin", + Service: consolev1.ConsolePluginService{ + Name: "plugin-service", + Namespace: "plugin-namespace", + Port: 8443, + BasePath: "/", + }, + }, + } + + _, err := client.ConsolePluginV1.Create( + ctx, plugin, metav1.CreateOptions{}, + ) + if err != nil { + t.Fatalf("failed to create plugin: %v", err) + } + + defer func() { + err := client.ConsolePluginV1.Delete( + ctx, pluginName, metav1.DeleteOptions{}, + ) + if err != nil { + t.Logf("cleanup warning: %v", err) + } + }() + + // Wait for plugin to be processed using framework helper + err = wait.Poll(1*time.Second, 1*time.Minute, func() (bool, error) { + deployment, err := framework.GetConsoleDeployment(client) + if err != nil { + return false, err + } + // Check for plugin in deployment + return containsPlugin(deployment, pluginName), nil + }) + if err != nil { + t.Fatalf("plugin not reflected in deployment: %v", err) + } +} +``` + +## Best Practices Checklist + +When creating e2e tests: + +- [ ] Use `framework.StandardSetup(t)` for test setup (not `MustNewClientset`) +- [ ] Use `framework.StandardCleanup(t, client)` with defer for cleanup +- [ ] Create context with timeout (`context.WithTimeout`) +- [ ] Use `retry.RetryOnConflict` for config updates +- [ ] Use `wait.Poll` for async operations (not `time.Sleep`) +- [ ] Use framework helpers (`GetConsoleDeployment`, `GetConsoleConfigMap`, etc.) +- [ ] Write helpful error messages with context +- [ ] Test both success and failure paths +- [ ] Verify operator responds to config changes +- [ ] Check ClusterOperator status conditions +- [ ] Handle `IsNotFound` errors in manual cleanup +- [ ] Use table-driven tests for multiple similar cases + +## Common Imports + +```go +import ( + "context" + "testing" + "time" + + configv1 "github.com/openshift/api/config/v1" + consolev1 "github.com/openshift/api/console/v1" + operatorv1 "github.com/openshift/api/operator/v1" + "github.com/openshift/console-operator/pkg/api" + "github.com/openshift/console-operator/test/e2e/framework" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" +) +``` + +## 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 + +## Output + +Generate complete, runnable test code with: +1. Proper imports +2. Framework setup +3. Resource creation +4. Cleanup logic +5. Wait patterns +6. Assertions +7. Helpful error messages +8. Comments explaining the test flow diff --git a/.claude/skills/e2e-test-review.md b/.claude/skills/e2e-test-review.md new file mode 100644 index 000000000..57d593931 --- /dev/null +++ b/.claude/skills/e2e-test-review.md @@ -0,0 +1,212 @@ +--- +name: e2e-test-review +description: Review e2e tests for best practices, proper cleanup, and wait patterns +tags: [review, testing, e2e] +--- + +# E2E Test Review Skill + +Review end-to-end tests in `test/e2e/` directory for correctness and best practices. + +## What to Check + +### 1. Test Structure +Tests should follow this pattern: +```go +func TestFeatureName(t *testing.T) { + client := framework.MustNewClientset(t, nil) + + // Setup + defer cleanupFunction(t, client) + + // Test logic with subtests + t.Run("subtest1", func(t *testing.T) { + // test code + }) +} +``` + +### 2. Table-Driven Tests +For testing multiple scenarios, use table-driven pattern: +```go +func TestMultipleScenarios(t *testing.T) { + testCases := []struct { + name string + input string + expected string + }{ + {name: "scenario1", input: "a", expected: "b"}, + {name: "scenario2", input: "c", expected: "d"}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + result := doSomething(tc.input) + if result != tc.expected { + t.Errorf("expected %v, got %v", tc.expected, result) + } + }) + } +} +``` + +### 3. Cleanup and Teardown +Always clean up created resources: +```go +// Use defer for cleanup +defer func() { + err := client.Delete(ctx, name, metav1.DeleteOptions{}) + if err != nil && !apierrors.IsNotFound(err) { + t.Logf("cleanup failed: %v", err) + } +}() +``` + +### 4. Wait Patterns +Use `wait.Poll` for eventual consistency: +```go +err := wait.Poll(1*time.Second, framework.AsyncOperationTimeout, func() (bool, error) { + deployment, err := client.AppsV1().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) +} +``` + +### 5. Retry Patterns +Use `retry.RetryOnConflict` for updates: +```go +err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + config, err := client.Get(ctx, name, metav1.GetOptions{}) + if err != nil { + return err + } + config.Spec.SomeField = "newValue" + _, err = client.Update(ctx, config, metav1.UpdateOptions{}) + return err +}) +``` + +### 6. Context Usage +Always use context with timeout: +```go +ctx, cancel := context.WithTimeout(context.Background(), framework.AsyncOperationTimeout) +defer cancel() +``` + +### 7. Framework Usage +Use test framework helpers: +```go +// Good - uses framework +client := framework.MustNewClientset(t, nil) +operatorConfig := framework.GetOperatorConfig(t, client) + +// Bad - manual setup +config, _ := clientcmd.BuildConfigFromFlags("", kubeconfig) +client, _ := kubernetes.NewForConfig(config) +``` + +### 8. Error Assertions +Be specific about expected errors: +```go +// Good - checks specific error +if !apierrors.IsNotFound(err) { + t.Fatalf("expected NotFound, got %v", err) +} + +// Bad - vague +if err != nil { + t.Fatal("got error") +} +``` + +### 9. Test Isolation +Tests should not depend on each other: +- Each test creates its own resources +- Cleanup removes all test resources +- No shared mutable state between tests + +### 10. Assertion Quality +```go +// Good - clear message with context +if actual != expected { + t.Errorf("plugin config mismatch: expected %v, got %v", expected, actual) +} + +// Bad - no context +if actual != expected { + t.Error("mismatch") +} +``` + +## Red Flags + +- Tests without cleanup (resource leaks) +- Missing `wait.Poll` when checking async operations +- Direct sleeps instead of wait conditions +- Tests that modify global state +- Tests without timeout contexts +- Assertions without helpful error messages +- Tests that require specific ordering to pass + +## E2E-Specific Patterns + +### Console Config Updates +```go +err := retry.RetryOnConflict(retry.DefaultBackoff, func() error { + config, err := client.ConfigV1().Consoles().Get( + ctx, "cluster", metav1.GetOptions{}, + ) + if err != nil { + return err + } + config.Spec.Customization = &configv1.ConsoleCustomization{ + Brand: configv1.BrandOKD, + } + _, err = client.ConfigV1().Consoles().Update( + ctx, config, metav1.UpdateOptions{}, + ) + return err +}) +``` + +### Plugin Testing +```go +// Create plugin +plugin := &consolev1.ConsolePlugin{ + ObjectMeta: metav1.ObjectMeta{Name: "test-plugin"}, + Spec: consolev1.ConsolePluginSpec{ + DisplayName: "Test Plugin", + Service: consolev1.ConsolePluginService{ + Name: "plugin-service", + Namespace: "plugin-ns", + Port: 8443, + }, + }, +} +defer cleanupPlugin(t, client, plugin.Name) +``` + +## Output Format + +For each issue: +- **Test**: Test function name +- **Issue**: What's wrong +- **Risk**: Why it matters +- **Fix**: How to improve + +## Example Review Comments + +**Critical**: Test has no cleanup - will leak ConsolePlugin resource. Add defer with cleanup. + +**Warning**: Using time.Sleep instead of wait.Poll. This makes test flaky and slower than necessary. + +**Suggestion**: Consider using table-driven tests to cover multiple plugin configurations. + +**Info**: Good use of retry.RetryOnConflict for config updates. 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..5a30c882f 100644 --- a/.coderabbit.yaml +++ b/.coderabbit.yaml @@ -1,5 +1,19 @@ # 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 tests use framework, wait.Poll, retry patterns +# /e2e-test-create - Scaffold new e2e tests with best practices +# /go-quality-review - Always applied; checks deprecated APIs, code smells + language: en-US inheritance: true @@ -15,27 +29,98 @@ reviews: review_details: true path_filters: - "!vendor/**" - - "!**/*_test.go" 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" + + Apply skills based on CODE PATTERNS, not just file paths: + + **Apply /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 + + **Apply /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 → Deployments → Routes + - Feature gate conditional logic + + **Apply /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. + + **Apply /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) + + **Apply /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 /e2e-test-create 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. + + **Apply /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 @@ -51,6 +136,7 @@ knowledge_base: - "ARCHITECTURE.md" - "CONVENTIONS.md" - "TESTING.md" + - ".claude/skills/*.md" linked_repositories: - repository: "openshift/console" instructions: >- diff --git a/.github/CODERABBIT_SETUP.md b/.github/CODERABBIT_SETUP.md new file mode 100644 index 000000000..a8401e01a --- /dev/null +++ b/.github/CODERABBIT_SETUP.md @@ -0,0 +1,122 @@ +# 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 test/e2e/**/*_test.go files | +| `e2e-test-create` | Scaffold new e2e tests | Creating new e2e tests | +| `go-quality-review` | Review code quality | Checking for deprecated APIs, code smells | + +### Using Skills + +In Claude Code CLI or IDE extensions: +```bash +/controller-review +/e2e-test-review +``` + +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.MustNewClientset(t, nil)` +- `wait.Poll` or `retry.RetryOnConflict` +- Console/operator CR manipulations + +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/)