-
Notifications
You must be signed in to change notification settings - Fork 194
ARO-25801: consolidate LLM workflow rules and local integration guidance #4739
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: master
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,45 @@ | ||
| --- | ||
| description: Ensure clean linear branch history before PR submission | ||
| alwaysApply: true | ||
| --- | ||
|
|
||
| # Clean Branch History | ||
|
|
||
| Before creating or updating a PR, verify the branch has a clean, reviewable history. | ||
|
|
||
| ## Pre-Push Checks | ||
|
|
||
| 1. **No merge commits** — rebase onto master instead: | ||
| ```bash | ||
| git log --oneline --merges master..HEAD | ||
| # Should produce no output | ||
| ``` | ||
|
|
||
| 2. **No fixup/WIP commits** — squash them before pushing: | ||
| ```bash | ||
| git log --oneline master..HEAD | grep -iE '(fixup|wip|squash|tmp|todo)' | ||
| # Should produce no output | ||
| ``` | ||
|
|
||
| 3. **No unrelated changes** — verify diff is scoped to the task: | ||
| ```bash | ||
| git diff --stat master..HEAD | ||
| ``` | ||
|
|
||
| 4. **Up to date with master** — check if a rebase is needed: | ||
| ```bash | ||
| git fetch origin master | ||
| git log --oneline HEAD..origin/master | ||
| ``` | ||
| If output exists, **ask the user before rebasing** — `git rebase origin/master` rewrites history and can cause conflicts. Never rebase autonomously. | ||
|
|
||
| ## Commit Message Guidelines | ||
|
|
||
| - Use conventional commit format: `type(scope): description` | ||
| - Types: `feat`, `fix`, `refactor`, `test`, `docs`, `chore`, `ci` | ||
| - Scope should match the package or area (e.g. `frontend`, `operator`, `api`) | ||
| - Body should explain **why**, not **what** (the diff shows what) | ||
|
|
||
| ## After Rebasing | ||
|
|
||
| Always re-run the validation pipeline (see `validation-pipeline` rule) after a rebase. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| --- | ||
| description: Checklist and focus areas when reviewing ARO-RP pull requests | ||
| alwaysApply: false | ||
| globs: | ||
| - "**/*.go" | ||
| --- | ||
|
|
||
| # Code Review | ||
|
|
||
| When reviewing a PR, check these areas systematically. | ||
|
|
||
| ## Structural Checks | ||
|
|
||
| 1. **Multi-module scope**: If the PR touches `pkg/api/`, verify tests were run separately (see `multi-module-awareness` rule). | ||
| 2. **Code style**: Verify imports, aliases, and banned packages conform (see `code-style` rule). | ||
|
|
||
| ## Architecture Checks | ||
|
|
||
| 3. **Async invariant**: Any frontend handler touching cluster state must go through CosmosDB async flow, never directly. | ||
| 4. **Runtime context**: No CI-only packages (`pkg/util/cluster`, `hack/`, `test/e2e`) imported from production packages. | ||
| 5. **Admin API pattern**: New admin endpoints should use the underscore pattern for testability. | ||
| 6. **VMSize types**: If VM sizes are involved, verify the correct type is used in context (see `multi-module-awareness` rule). | ||
|
|
||
| ## Comment–Code Coherence | ||
|
|
||
| 7. **Comments must match code behaviour**: When a log message, error string, or doc comment describes an action (e.g. "Uncordoning node"), the called function must obviously perform that action. Boolean-flag APIs like `CordonNode(ctx, name, false)` to mean "uncordon" are fragile — wrap them in named helpers (`uncordonNode`) or use named constants so the intent is visible at the call site. | ||
| 8. **Function names must match semantics**: If a function is named `getClusterMachines` but it only returns control plane machines, the name is misleading. Use `getControlPlaneMachines` or add a wrapper that clarifies the filtering. | ||
| 9. **Error wrapping must not hide types**: Do NOT double-wrap errors in `api.CloudError` or `fmt.Errorf` when the original error is already a `CloudError` — `adminReply()` relies on type assertions to set the HTTP status code (see `code-style` rule). | ||
|
|
||
| ## Test Checks | ||
|
|
||
| 10. **Coverage**: Are the changed code paths tested? | ||
| 11. **Mock usage**: Mocks should be generated via `mockgen` (`//go:generate`), not hand-written. | ||
|
|
||
| ## PR Hygiene | ||
|
|
||
| 12. **Clean history**: No merge commits, fixup commits, or WIP commits (see `clean-history` rule). | ||
| 13. **Scoped changes**: Diff should only contain task-related files. | ||
| 14. **Swagger**: If API types changed, `make generate-swagger` output should be included. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| --- | ||
| description: Code style conventions enforced by CI — imports, formatting, patterns | ||
| alwaysApply: true | ||
| --- | ||
|
|
||
| # Code Style | ||
|
|
||
| ## Formatting | ||
|
|
||
| Always use `make fmt`, never `gofmt`. The fmt target runs `golangci-lint fmt` which applies **gci** (import ordering) + **gofumpt** (stricter gofmt). | ||
|
|
||
| ## Import Ordering (9 tiers, enforced by gci) | ||
|
|
||
| 1. Standard library | ||
| 2. Blank imports | ||
| 3. Dot imports | ||
| 4. Third-party (default) | ||
| 5. `k8s.io/*` | ||
| 6. `sigs.k8s.io/*` | ||
| 7. `github.com/Azure/*` | ||
| 8. `github.com/openshift/*` | ||
| 9. Local module packages | ||
|
|
||
| ## Required Import Aliases | ||
|
|
||
| The `importas` linter enforces specific aliases — CI will reject wrong ones. Key examples: | ||
|
|
||
| - `kerrors` -> `k8s.io/apimachinery/pkg/api/errors` | ||
| - `metav1` -> `k8s.io/apimachinery/pkg/apis/meta/v1` | ||
| - `arov1alpha1` -> ARO operator v1alpha1 API | ||
| - `ctrl` -> `sigs.k8s.io/controller-runtime` | ||
| - `kruntime` -> `k8s.io/apimachinery/pkg/runtime` | ||
|
|
||
| See `.golangci.yml` for the full list of ~40+ enforced aliases. | ||
|
|
||
| ## Banned Packages | ||
|
|
||
| The `depguard` linter blocks these — use `pkg/util/pointerutils` instead: | ||
| - `github.com/Azure/go-autorest/autorest/to` | ||
| - `k8s.io/utils/ptr` | ||
|
|
||
| ## Error Handling | ||
|
|
||
| - Wrap: `fmt.Errorf("context: %w", err)` | ||
| - Check: `errors.Is()` / `errors.As()` | ||
| - **Do NOT wrap errors in `api.CloudError` if you're not adding context** — `adminReply()` has a type-switch that converts `kerrors.APIStatus`, `*api.CloudError`, `statusCodeError`, and bare `error` into the correct HTTP response. Double-wrapping a `CloudError` inside another `CloudError` (or wrapping it with `fmt.Errorf`) defeats this mechanism by hiding the original type. When the underscore method returns an error already of the right type, return it directly. | ||
|
|
||
| ## Package Rules | ||
|
|
||
| - No files directly in `pkg/util/` — always use a subpackage. | ||
| - No filenames containing colons (Windows compat). | ||
|
|
||
| ## Admin API Handler Pattern | ||
|
|
||
| New admin endpoints must use the "underscore pattern" — main handler extracts HTTP params, calls `_methodName()` with raw values for testability: | ||
|
|
||
| ```go | ||
| func (f *frontend) postAdminFoo(w http.ResponseWriter, r *http.Request) { | ||
| err := f._postAdminFoo(log, ctx, r) | ||
| adminReply(log, w, nil, nil, err) | ||
| } | ||
|
|
||
| func (f *frontend) _postAdminFoo(log *logrus.Entry, ctx context.Context, r *http.Request) error { | ||
| // Business logic here — testable without HTTP mocking | ||
| } | ||
| ``` | ||
|
|
||
| Register routes in `pkg/frontend/frontend.go`. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| --- | ||
| description: Always use Makefile targets instead of ad-hoc commands | ||
| alwaysApply: true | ||
| --- | ||
|
|
||
| # Makefile First | ||
|
|
||
| The root `Makefile` is the single source of truth for all quality gates, builds, and CI operations. Never improvise shell commands for operations that have a Makefile target. | ||
|
|
||
| ## Formatting & Linting | ||
|
|
||
| - `make fmt` — Format BOTH Go modules (golangci-lint: gci + gofumpt). **Never use `gofmt` directly.** | ||
| - `make lint-go` — Run all linters (both modules) | ||
| - `make lint-go-fix` — Lint with `--fix` (both modules) | ||
| - `make validate-imports` — Verify import ordering matches CI expectations | ||
|
|
||
| ## Testing | ||
|
|
||
| - `make unit-test-go` — Unit tests for **root module only** (see `multi-module-awareness` rule for the `pkg/api` trap) | ||
| - `go test -v ./pkg/frontend/... -run TestSpecificFunction` — Run a single test | ||
| - `make test-go` — Full validation: generate + build-all + validate-go + lint-go + unit-test-go | ||
|
|
||
| ## Code Generation | ||
|
|
||
| - `make generate` — Run all `//go:generate` directives + swagger | ||
| - `make generate-swagger` — Regenerate swagger specs only | ||
| - `make go-tidy` — `go mod tidy` for BOTH modules | ||
|
|
||
| ## Building | ||
|
|
||
| - `make aro` — Build the ARO RP binary | ||
| - `make build-all` — Build all Go binaries | ||
| - `make build-portal` — Build the portal frontend (npm) | ||
|
|
||
| ## Local Development | ||
|
|
||
| - `make runlocal-rp` — Run RP locally | ||
| - `make runlocal-monitor` — Run monitor locally | ||
| - `make runlocal-portal` — Run portal locally | ||
|
|
||
| ## Discovery | ||
|
|
||
| Run `make help` to list all available targets with descriptions. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| --- | ||
| description: Two Go modules — root and pkg/api — with critical testing and build implications | ||
| alwaysApply: true | ||
| --- | ||
|
|
||
| # Multi-Module Awareness | ||
|
|
||
| ARO-RP has two Go modules. Getting this wrong causes silent test gaps or CI failures. | ||
|
|
||
| ## Module Layout | ||
|
|
||
| | Module | go.mod | Path | | ||
| |--------|--------|------| | ||
| | Root | `go.mod` | `github.com/Azure/ARO-RP` | | ||
| | API | `pkg/api/go.mod` | `github.com/Azure/ARO-RP/pkg/api` | | ||
|
|
||
| Root imports API via a `replace` directive. Go's `./...` wildcard **excludes** subdirectories with their own `go.mod`. | ||
|
|
||
| ## Critical Implications | ||
|
|
||
| 1. **`go test ./...` from root does NOT test `pkg/api/`** — you must `cd pkg/api && go test ./...` separately. | ||
| 2. **`go mod tidy` from root does NOT update `pkg/api/go.mod`** — use `make go-tidy` which handles both. | ||
| 3. **`make fmt` and `make lint-go-fix` handle both modules** — these are safe to run from root. | ||
|
|
||
| ## Adding Dependencies | ||
|
|
||
| - If a new dependency is used in `pkg/api/`, add it to `pkg/api/go.mod`. | ||
| - If used in the root module, add it to the root `go.mod`. | ||
| - If used in both, it must appear in both. Always run `make go-tidy` after changes. | ||
|
|
||
| ## Three VMSize Types (NOT interchangeable) | ||
|
|
||
| 1. `api.VMSize` — internal, stored in CosmosDB | ||
| 2. `vms.VMSize` — utility type with metadata, used by admin API and validate | ||
| 3. Local `VMSize` in each `pkg/api/v*/` — external ARM-facing, matches swagger | ||
|
|
||
| Conversion files (`_convert.go`) bridge them with explicit casts. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| --- | ||
| description: PR creation, CI monitoring, and merge workflow for ARO-RP | ||
| alwaysApply: true | ||
| --- | ||
|
|
||
| # PR Lifecycle | ||
|
|
||
| ## Creating | ||
|
|
||
| 1. Run the full validation pipeline (see `validation-pipeline` rule). | ||
| 2. Verify clean branch history (see `clean-history` rule). | ||
| 3. **ASK the user for approval before pushing or creating the PR.** Never push or open a PR autonomously. | ||
| 4. Push: `git push -u origin HEAD` | ||
| 5. Open PR: `gh pr create` with summary and test plan. | ||
|
|
||
| ## CI Monitoring | ||
|
|
||
| ARO-RP has multiple CI systems (GitHub Actions + Azure Pipelines / Prow). Key workflows: | ||
|
|
||
| - **ci-go**: vendor-check, generate-check, golangci-lint, validate-go | ||
| - **ci-python**: Python linting and tests | ||
| - **CodeQL**: Static analysis (Go, JS, Python) | ||
| - **Test coverage**: 6 parallel suites (cmd, pkg-api, pkg-frontend, pkg-operator, pkg-util, pkg-other) | ||
| - **ci/prow/images**: OpenShift CI image build | ||
|
|
||
| ### Monitoring Steps | ||
|
|
||
| 1. Poll: `gh pr checks <PR-NUMBER>` until all checks complete. | ||
| 2. On failure, read logs: `gh run view <RUN-ID> --job <JOB-ID> --log-failed` | ||
| 3. Fix in the worktree, commit, push, re-monitor. | ||
| 4. If failure is unrelated (e.g. flaky infra test), note it and proceed. | ||
|
|
||
| ## Merging | ||
|
|
||
| **ALWAYS ask the user for explicit approval before merging.** Never merge autonomously. | ||
|
|
||
| - Squash merge: `gh pr merge <number> --squash --body "..."` | ||
| - After merge: `git checkout master && git pull` |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,45 @@ | ||||||
| --- | ||||||
| description: Required reading and approval gates for sensitive operations | ||||||
| alwaysApply: true | ||||||
| --- | ||||||
|
|
||||||
| # Safety Rails | ||||||
|
|
||||||
| ## Required Reading Before Changes | ||||||
|
|
||||||
| | Trigger | Required reading | | ||||||
| |---------|-----------------| | ||||||
| | Modifying `pkg/api/v*` types | `docs/agent-guides/api-type-system.md` | | ||||||
| | Adding/changing VM sizes | `docs/agent-guides/azure-product-constraints.md` | | ||||||
| | Touching `pkg/cluster`, `pkg/util/cluster`, or `pkg/deploy` | `docs/agent-guides/package-deployment-context.md` | | ||||||
| | Changing Makefile, CI, or build targets | `docs/agent-guides/multi-module-build.md` | | ||||||
|
|
||||||
| ## Architecture Invariant | ||||||
|
|
||||||
| Cluster mutations (PUT/DELETE) are **async**: Frontend writes to CosmosDB with non-terminal state, Backend polls and processes, then updates with terminal state. **Never bypass this** — frontend handlers must NOT perform cluster operations directly. | ||||||
|
|
||||||
| ## Where Code Runs | ||||||
|
|
||||||
| | Runtime context | Packages | | ||||||
| |----------------|----------| | ||||||
| | RP control plane (Azure VMSS) | `pkg/frontend`, `pkg/backend`, `pkg/cluster`, `pkg/monitor`, `pkg/gateway`, `pkg/portal` | | ||||||
| | Customer OpenShift cluster | `pkg/operator/controllers` (26 controllers) | | ||||||
|
||||||
| | Customer OpenShift cluster | `pkg/operator/controllers` (26 controllers) | | |
| | Customer OpenShift cluster | `pkg/operator/controllers` | |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| --- | ||
| description: Ordered validation pipeline — format, lint, unit test, integration test | ||
| alwaysApply: true | ||
| --- | ||
|
|
||
| # Validation Pipeline | ||
|
|
||
| Run validation steps in this exact order. **Stop on first failure** — do not proceed to the next stage until the current one passes. | ||
|
|
||
| ## Stage 1: Format | ||
|
|
||
| ```bash | ||
| make fmt | ||
| ``` | ||
|
|
||
| Check for unstaged changes after formatting: | ||
|
|
||
| ```bash | ||
| git diff --quiet || echo "Formatting produced changes — stage and review them" | ||
| ``` | ||
|
|
||
| ## Stage 2: Lint | ||
|
|
||
| ```bash | ||
| make lint-go | ||
| ``` | ||
|
|
||
| See the `code-style` rule for details on common lint failures and enforced conventions. | ||
|
|
||
| ## Stage 3: Unit Tests | ||
|
|
||
| ```bash | ||
| make unit-test-go | ||
| ``` | ||
|
|
||
| If you changed anything under `pkg/api/`, also run: | ||
|
|
||
| ```bash | ||
| cd pkg/api && go test ./... | ||
| ``` | ||
|
Comment on lines
+22
to
+40
|
||
|
|
||
| See the `multi-module-awareness` rule for why this separate step is required. | ||
|
|
||
| ## Stage 4: Full Validation (before pushing) | ||
|
|
||
| ```bash | ||
| make test-go | ||
| ``` | ||
|
|
||
| This runs the full pipeline: `generate` -> `build-all` -> `validate-go` -> `lint-go` -> `unit-test-go`. | ||
|
|
||
| ## When to Run Each Stage | ||
|
|
||
| | Change type | Minimum validation | | ||
| |-------------|-------------------| | ||
| | Go code in root module | Stages 1-3 | | ||
| | Go code in `pkg/api/` | Stages 1-3 + `cd pkg/api && go test ./...` | | ||
| | Swagger-facing types | Stages 1-3 + `make generate-swagger` | | ||
| | Makefile / CI changes | Stage 4 (full `make test-go`) | | ||
| | Code generation templates | `make generate` then Stages 1-3 | | ||
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.
make lint-goonly runs golangci-lint in the root module (it does notcd pkg/api), so describing it as “(both modules)” is inaccurate and can lead to missed lint failures inpkg/api/. Please either clarify thatlint-gois root-module only, or point to a command/target that also lintspkg/api/(e.g.,make lint-go-fixor an explicitcd pkg/api && golangci-lint run ...).