-
Notifications
You must be signed in to change notification settings - Fork 75
NO-JIRA: Refactor AGENTS.md to be minimal with references #587
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,56 @@ | ||||||||||
| # Code Structure | ||||||||||
|
|
||||||||||
| ## Architecture | ||||||||||
|
|
||||||||||
| The operator consists of four main binaries: | ||||||||||
|
|
||||||||||
| 1. **capi-operator** (`cmd/capi-operator/`) - Manages installation of Cluster API components and providers. Runs the ClusterOperator, Revision, and Installer controllers. | ||||||||||
| 2. **capi-controllers** (`cmd/capi-controllers/`) - Manages CAPI cluster resources, infrastructure integration, secret synchronization, and kubeconfig management. Runs the CoreCluster, InfraCluster, SecretSync, and Kubeconfig controllers plus webhooks. | ||||||||||
| 3. **machine-api-migration** (`cmd/machine-api-migration/`) - Handles migration between Machine API and Cluster API resources. Only runs when the MachineAPIMigration feature gate is enabled. Currently supports AWS and OpenStack platforms. | ||||||||||
| 4. **crd-compatibility-checker** (`cmd/crd-compatibility-checker/`) - Validates CRD compatibility requirements, runs object validation/pruning webhooks, and installs static resources for the compatibility requirements system. | ||||||||||
|
|
||||||||||
| The repository also includes: | ||||||||||
|
|
||||||||||
| 5. **manifests-gen** (`manifests-gen/`) - Standalone tool that generates admission policy profiles from upstream Cluster API provider manifests for embedding into the operator image. | ||||||||||
|
|
||||||||||
| ## Key Controllers | ||||||||||
|
|
||||||||||
| ### capi-operator Controllers | ||||||||||
| - **ClusterOperator Controller** (`pkg/controllers/clusteroperator/`) - Manages the operator's ClusterOperator status resource. Always runs, even on unsupported platforms. | ||||||||||
| - **Revision Controller** (`pkg/controllers/revision/`) - Manages OLM revision resources for tracking installed provider versions and triggering upgrades. | ||||||||||
| - **Installer Controller** (`pkg/controllers/installer/`) - Handles installation and lifecycle management of CAPI components and providers using the boxcutter framework. | ||||||||||
|
|
||||||||||
| ### capi-controllers Controllers | ||||||||||
| - **Core Cluster Controller** (`pkg/controllers/corecluster/`) - Manages CAPI Cluster resources representing the OpenShift cluster | ||||||||||
| - **Infra Cluster Controller** (`pkg/controllers/infracluster/`) - Manages infrastructure-specific cluster resources (AWS, Azure, GCP, etc.) | ||||||||||
| - **Secret Sync Controller** (`pkg/controllers/secretsync/`) - Synchronizes secrets between MAPI and CAPI namespaces | ||||||||||
| - **Kubeconfig Controller** (`pkg/controllers/kubeconfig/`) - Manages kubeconfig secrets for cluster access | ||||||||||
|
|
||||||||||
| ### crd-compatibility-checker Controllers | ||||||||||
| - **CRD Compatibility Controller** (`pkg/controllers/crdcompatibility/`) - Reconciles CompatibilityRequirement resources and validates CRD create/update/delete operations via webhooks. Includes object validation and pruning sub-controllers. | ||||||||||
| - **Static Resource Installer Controller** (`pkg/controllers/staticresourceinstaller/`) - Installs static Kubernetes resources from embedded asset files on startup. | ||||||||||
|
|
||||||||||
| ### machine-api-migration Controllers | ||||||||||
| - **Machine Migration Controller** (`pkg/controllers/machinemigration/`) - Handles handover of AuthoritativeAPI and object pausing for machine migration | ||||||||||
| - **MachineSet Migration Controller** (`pkg/controllers/machinesetmigration/`) - Handles handover of AuthoritativeAPI and object pausing for machineset migration | ||||||||||
| - **Machine Sync Controller** (`pkg/controllers/machinesync/`) - Synchronizes individual machine related resources between APIs | ||||||||||
| - **MachineSet Sync Controller** (`pkg/controllers/machinesetsync/`) - Synchronizes machineset related objects between APIs | ||||||||||
|
Comment on lines
+36
to
+37
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hyphenate compound modifiers. Lines 36 and 37 use "machine related" and "machineset related" as compound modifiers before nouns. According to English style conventions, these should be hyphenated: "machine-related resources" and "machineset-related objects". 📝 Proposed fix-- **Machine Sync Controller** (`pkg/controllers/machinesync/`) - Synchronizes individual machine related resources between APIs
+- **Machine Sync Controller** (`pkg/controllers/machinesync/`) - Synchronizes individual machine-related resources between APIs
-- **MachineSet Sync Controller** (`pkg/controllers/machinesetsync/`) - Synchronizes machineset related objects between APIs
+- **MachineSet Sync Controller** (`pkg/controllers/machinesetsync/`) - Synchronizes machineset-related objects between APIs📝 Committable suggestion
Suggested change
🧰 Tools🪛 LanguageTool[grammar] ~36-~36: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) [grammar] ~37-~37: Use a hyphen to join words. (QB_NEW_EN_HYPHEN) 🤖 Prompt for AI Agents |
||||||||||
|
|
||||||||||
| ### Shared Packages | ||||||||||
| - **Sync Common** (`pkg/controllers/synccommon/`) - Shared apply-configuration helpers and migration status logic used by the machine and machineset sync/migration controllers | ||||||||||
|
|
||||||||||
| ## Conversion Framework | ||||||||||
| - **MAPI to CAPI Conversion** (`pkg/conversion/mapi2capi/`) - Converts MAPI resources to CAPI (supports AWS, OpenStack) | ||||||||||
| - **CAPI to MAPI Conversion** (`pkg/conversion/capi2mapi/`) - Converts CAPI resources to MAPI (supports AWS, OpenStack) | ||||||||||
| - **Conversion Utilities** (`pkg/conversion/util/`, `pkg/conversion/consts/`) - Shared constants and helper functions | ||||||||||
| - **Conversion Test Utilities** (`pkg/conversion/test/`) - Fuzz testing and test matchers for conversion logic | ||||||||||
|
|
||||||||||
| ## File Structure | ||||||||||
| - `manifests/` - OpenShift manifests for operator deployment | ||||||||||
| - `capi-operator-manifests/` - Upstream CAPI provider manifests consumed by manifests-gen | ||||||||||
| - `admission-policies/` - Kustomize overlays for admission policy profiles (default, AWS) | ||||||||||
| - `manifests-gen/` - Tool for generating admission policy profiles | ||||||||||
| - `hack/` - Development and testing scripts | ||||||||||
| - `docs/controllers/` - Detailed controller documentation | ||||||||||
| - `e2e/` - End-to-end tests for each supported platform | ||||||||||
| - `vendor/` - Vendored dependencies (use `make vendor` to update) | ||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # Style Guide | ||
|
|
||
| ## Coding Style | ||
| - Use early returns | ||
| - Descriptive names | ||
| - Helper functions over inline code | ||
| - Minimal comments (only for non-obvious decisions) | ||
| - Simple code over complex language features | ||
| - For user-facing text like logs and errors, use "Cluster API" and "Machine API". For code and internal identifiers, use "CAPI" and "MAPI". |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| # Tasks | ||
|
|
||
| ## Essential Commands | ||
| ```bash | ||
| # Build and test | ||
| make build # Build all binaries | ||
| make test # Run verification (fmt + lint) then unit tests | ||
| make unit # Run unit tests with coverage | ||
| make verify # Run fmt, lint, and verify-ocp-manifests | ||
| make lint # Run linting (golangci-lint) | ||
| make fmt # Format code (golangci-lint --fix) | ||
| make vendor # Vendor dependencies | ||
| make ocp-manifests # Generate admission policy profiles | ||
| ``` | ||
|
|
||
| ## Running Tests | ||
|
|
||
| **Do not use `go test` or `ginkgo` directly.** Tests use `envtest` which requires `KUBEBUILDER_ASSETS` | ||
| to point at downloaded API server and etcd binaries. The Makefile handles this: `make unit` depends on | ||
| the `.localtestenv` target (which runs `setup-envtest` to download binaries and writes their path to | ||
| `.localtestenv`), and `hack/test.sh` sources that file before invoking ginkgo. Running `go test` | ||
| directly will fail because the envtest `Environment` cannot locate the binaries. | ||
|
|
||
| ```bash | ||
| make unit # All unit tests | ||
| make unit TEST_DIRS="./pkg/controllers/installer/..." # Specific package | ||
| make unit TEST_DIRS="./pkg/controllers/machinesync/..." # Another specific package | ||
| ``` | ||
|
|
||
| **Important:** Ginkgo functional tests are slow and produce verbose output that will exceed | ||
| context limits. Always redirect output to a log file and use multi-pass processing: | ||
| ```bash | ||
| make unit TEST_DIRS="./pkg/..." 2>&1 | tee /tmp/test-output.log | ||
| # Then check results: | ||
| tail -20 /tmp/test-output.log # Summary | ||
| grep -E 'FAIL|PASSED' /tmp/test-output.log # Pass/fail status | ||
| grep 'FAIL' /tmp/test-output.log # Find failures | ||
| ``` | ||
|
|
||
| ### Default ginkgo arguments | ||
| The default ginkgo args in `hack/test.sh` are: | ||
| - `-r -v -p --randomize-all --randomize-suites --keep-going --race --trace --timeout=${TIMEOUT}` | ||
| - The timeout defaults to `20m` for unit tests (set by the Makefile) and `120m` for e2e tests. | ||
| - In CI (`OPENSHIFT_CI=true`), `-p` is replaced with `--procs=4`. | ||
|
|
||
| Prefer using `GINKGO_EXTRA_ARGS` to pass additional arguments to ginkgo. Use `GINKGO_ARGS` when you need to override the default values entirely. | ||
|
|
||
| ### Focused Testing | ||
| ```go | ||
| // Focus specific tests (REMOVE before committing!) | ||
| FIt("test name", func() { /* test */ }) | ||
| FContext("context name", func() { /* tests */ }) | ||
| ``` | ||
|
|
||
| ### Test Environment | ||
| - Each controller has a `suite_test.go` that bootstraps an `envtest.Environment` | ||
| - See "Running Tests" above for why `make unit` is required |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,159 @@ | ||
| # Testing | ||
|
|
||
| ## Choosing the right test level | ||
|
|
||
| Pick the cheapest level that can adequately cover the behaviour. Do not escalate without reason. | ||
|
|
||
| **Unit test** (no client or fake client, no envtest) — for pure logic, conversions, single-function behaviour, error paths that don't depend on server-side behaviour. Use `fake.NewClientBuilder()` only when the test doesn't depend on realistic API server responses (field defaulting, status subresource semantics, conflict errors, SSA merge, etc.). These run in milliseconds. | ||
|
|
||
| **Integration test** (envtest) — for anything that interacts with a Kubernetes API: controller reconciliation loops, multi-resource interactions, watching, status updates, and any scenario where fake client behaviour diverges from a real API server. Prefer envtest over fakes when in doubt — faking accurately is hard and flaky fakes waste more time than the slower test. Use `pkg/test.StartEnvTest()` in `suite_test.go`. These run in seconds. | ||
|
|
||
| **E2E test** (`e2e/`) — only for behaviour that requires real infrastructure: actual machine provisioning, cloud API interactions, cross-component migration flows. These run in minutes. | ||
|
|
||
| Rules of thumb: | ||
| - If you're testing "does this function return the right value/error" and it doesn't need a client → unit test. | ||
| - If you're testing any controller or client interaction → integration test (envtest). | ||
| - If you're testing "does a real machine appear in the cloud" → e2e test. | ||
| - If envtest can reproduce the scenario, do not write an e2e test. | ||
|
|
||
| ## Use existing shared helpers | ||
|
|
||
| Before writing new test utilities, builders, matchers, or setup code, search the repo for existing ones — particularly in `pkg/test/`, `pkg/conversion/test/`, `pkg/admissionpolicy/testutils/`, `e2e/framework/`, and the vendored `testutils/resourcebuilder/` package. Do not duplicate what already exists. If you need a variant, extend the existing helper rather than creating a parallel one. | ||
|
|
||
| ## Ginkgo/Gomega Best Practices | ||
|
|
||
| Use **Ginkgo/Gomega** framework and prefer built-in features over custom implementations: | ||
| - Use `DescribeTable` with `Entry` for table-driven tests instead of manual loops | ||
| - Use `HaveField`, `HaveValue`, `HaveKey` for struct/map assertions instead of manual field checks | ||
| - Use `ConsistOf` for unordered slice matching instead of sorting + `Equal` | ||
| - Use `MatchError` for error checking instead of string contains | ||
| - Use `BeNumerically` for numeric comparisons instead of manual range checks | ||
|
|
||
| ## Test Organization | ||
|
|
||
| - **Nested Contexts**: Organize related test scenarios with nested `Context()` blocks | ||
| ```go | ||
| Context("when migrating from MachineAPI to ClusterAPI", func() { | ||
| Context("when status is not paused", func() { | ||
| // Test cases | ||
| }) | ||
| }) | ||
| ``` | ||
| - **Descriptive test names**: Describe expected behaviour, not implementation details. Use "should..." format: | ||
| ```go | ||
| // good — describes behaviour | ||
| It("should reject machines with duplicate provider IDs", func() { ... }) | ||
|
|
||
| // bad — describes implementation | ||
| It("should return an error from validateProviderID", func() { ... }) | ||
| ``` | ||
| - **Use `By()` for test steps**: Document distinct phases within a test with `By("Setting up namespaces for the test")` | ||
|
|
||
| ## Async Assertions with Komega | ||
|
|
||
| Use **Komega** for Kubernetes object assertions: | ||
| ```go | ||
| // Use komega.Object for async assertions | ||
| Eventually(k.Object(myResource)).Should(HaveField("ObjectMeta.ResourceVersion", Equal(expectedRV))) | ||
|
|
||
| // Update resources with komega helpers | ||
| Eventually(k.UpdateStatus(myResource, func() { | ||
| myResource.Status.SomeField = "value" | ||
| })).Should(Succeed()) | ||
| ``` | ||
|
|
||
| ## Resource Management | ||
|
|
||
| - **Resource builders**: Use `cluster-api-actuator-pkg/testutils/resourcebuilder` for creating test objects. | ||
| Builders are organized by API group (e.g., `machine/v1beta1`, `cluster-api/core/v1beta2`, `cluster-api/infrastructure/v1beta2`, `config/v1`, `core/v1`). | ||
| ```go | ||
| mapiMachine = mapiMachineBuilder. | ||
| WithNamespace(namespace). | ||
| WithName("foo"). | ||
| WithAuthoritativeAPI(machinev1beta1.MachineAuthorityMachineAPI). | ||
| Build() | ||
| ``` | ||
| - **Standard cleanup**: Use `testutils.CleanupResources()` in AfterEach (from `cluster-api-actuator-pkg/testutils`) | ||
| ```go | ||
| testutils.CleanupResources(Default, ctx, cfg, k8sClient, namespace, | ||
| &machinev1beta1.Machine{}, | ||
| &clusterv1.Machine{}, | ||
| ) | ||
| ``` | ||
|
|
||
| ## Assertions | ||
|
|
||
| Prefer precise matchers over multiple loose ones. Combine related assertions into a single matcher (e.g., `SatisfyAll`, `ConsistOf`). With `Eventually`, each separate assertion polls with its own timeout — multiple assertions checking the same object multiply the wait time on failure. | ||
|
|
||
| ```go | ||
| // good — single assertion, exact match | ||
| Expect(err).To(MatchError(expectedErr)) | ||
|
|
||
| // bad — two assertions, string matching | ||
| Expect(err).To(HaveOccurred()) | ||
| Expect(err).To(MatchError(ContainSubstring("connection refused"))) | ||
| ``` | ||
|
|
||
| When an expected error is reused across multiple test cases, declare it as a variable rather than duplicating the literal. | ||
|
|
||
| - **Complex assertions**: Combine matchers with `SatisfyAll` | ||
| ```go | ||
| Eventually(komega.Object(resource)).Should(SatisfyAll( | ||
| HaveField("Status.AuthoritativeAPI", Equal(expected)), | ||
| HaveField("Status.SynchronizedGeneration", BeZero()), | ||
| )) | ||
| ``` | ||
| - **Checking absence**: Use `ShouldNot` with appropriate matchers | ||
| ```go | ||
| Eventually(komega.Object(resource)).ShouldNot( | ||
| HaveField("ObjectMeta.Annotations", ContainElement(HaveKeyWithValue(key, value)))) | ||
| ``` | ||
| - **Nested field checks**: Chain `HaveField` for nested assertions | ||
| ```go | ||
| HaveField("Status.Conditions", ContainElement(SatisfyAll( | ||
| HaveField("Type", Equal("Paused")), | ||
| HaveField("Status", Equal(corev1.ConditionTrue)), | ||
| ))) | ||
| ``` | ||
|
|
||
| ## Debuggable Failures | ||
|
|
||
| Every test failure must be debuggable from the output alone — without reading test source code. | ||
|
|
||
| **Assertion messages.** If a failure's stack trace and default matcher output wouldn't tell you what went wrong, add a description. This applies especially to generic matchers like `BeNil()`, `BeTrue()`, `HaveLen()` where the default output doesn't convey intent. | ||
|
|
||
| ```go | ||
| // good — failure output explains the scenario | ||
| Expect(transport).To(BeNil(), "expected nil transport when additionalTrustedCA is not set") | ||
|
|
||
| // bad — failure output is just "expected nil, got &http.Transport{...}" | ||
| Expect(transport).To(BeNil()) | ||
| ``` | ||
|
|
||
| **Stack traces.** Do not call `Expect`, `Fail`, or panic from helper functions — failures will point at the helper, not the test that called it. Return errors to the calling test instead. | ||
|
|
||
| If assertions inside a helper are unavoidable, use `GinkgoHelper()` so the stack trace shows the caller: | ||
|
|
||
| ```go | ||
| func expectResourceReady(obj client.Object) { | ||
| GinkgoHelper() | ||
| Expect(obj.GetAnnotations()).To(HaveKey("ready")) | ||
| } | ||
| ``` | ||
|
|
||
| ## No Sleeps, No Timeout Bumps | ||
|
|
||
| In event-driven systems, tests should wait for conditions, not for time to pass. | ||
|
|
||
| - **Never use `time.Sleep()`**. Use `Eventually` with a condition that checks the actual state you're waiting for. | ||
| - **Do not bump `Eventually` timeouts to fix flaky tests.** A flaky test means the test is waiting for the wrong condition or the code has a race. Fix the root cause. | ||
| - **`Consistently` durations should be meaningful.** Too-short durations prove nothing — the condition might change immediately after. Use a duration long enough to cover at least a few reconciliation cycles. | ||
|
|
||
| ```go | ||
| // good — waits for the actual state change | ||
| Eventually(komega.Object(machine)).Should(HaveField("Status.Phase", Equal("Running"))) | ||
|
|
||
| // bad — arbitrary sleep hoping the controller has finished | ||
| time.Sleep(5 * time.Second) | ||
| Expect(machine.Status.Phase).To(Equal("Running")) | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`.agents/` is not a standard directory — consider hierarchical `AGENTS.md` files instead
The established convention for agent instructions is hierarchical `AGENTS.md` files placed in subdirectories. Agents auto-read the nearest `AGENTS.md` in the directory tree — the closest one takes precedence, and each subdirectory can ship tailored instructions. The OpenAI repo has 88 of them. This is supported natively by Claude Code, Codex, Cursor, Copilot, Jules, and others, and is now governed by the Agentic AI Foundation under the Linux Foundation.
There is a community proposal for a `.agent` directory (singular, different structure), but it's at the discussion stage and not adopted by any tool. The `.agents/reference/` directory used here doesn't match the standard, doesn't match the proposal, and won't be auto-discovered by any agent tooling.
These files will only be read if an agent actively decides to follow the links in the root AGENTS.md — which is not guaranteed, especially for quick fixes or tasks where the agent doesn't think to check conventions.
Suggestion: Place this content in `AGENTS.md` files in relevant subdirectories instead (e.g., `pkg/AGENTS.md`, `e2e/AGENTS.md`). This gets the same "thin root, detailed when needed" outcome while using the standard mechanism that tools already auto-discover.