diff --git a/.claude/SKILLS.md b/.claude/SKILLS.md index e2dbf31b9352..8d02ee1cd2b8 100644 --- a/.claude/SKILLS.md +++ b/.claude/SKILLS.md @@ -32,6 +32,23 @@ This directory contains Claude Code skills that are automatically applied when w - Includes proper attribution footers - Follows conventional commits specification +### Restructure Commits + +**Location:** `skills/restructure-commits/` (canonical, harness-neutral); `.claude/skills/restructure-commits/` and `.cursor/rules/restructure-commits.mdc` are thin adapters + +**Description:** Model-neutral procedure for rewriting a branch into component-based commits (API, Dependencies, Vendor, CLI, HO, CPO, E2E, Docs, Tooling) without changing its file content. + +**Use when:** + +- User asks to "redo commits", "restructure commits", "squash by component", or "organize commits" +- Preparing a branch for PR review or cleaning history after review feedback + +**Covers:** + +- Ownership rules in `ownership.json`, generated from repository facts by `generate_ownership.py` (`make update`) +- `commit_layout.py` to plan, stage, and verify commits; `make verify-commit-layout` is the completion gate +- Safe preflight: PR base resolution, backup branch, tree snapshot, and `--force-with-lease` against the recorded SHA + ### Effective Go **Location:** `.claude/skills/effective-go/` @@ -224,31 +241,6 @@ Skills are automatically invoked by Claude based on context. You don't need to d Commands are manually invoked using `/command-name` syntax. -### Restructure Commits - -**Location:** `.claude/commands/restructure-commits.md` - -**Description:** Reorganizes all commits on a feature branch into logical, component-based commits that match HyperShift's architecture. - -**Usage:** - -``` -/restructure-commits -``` - -**Use when:** - -- User asks to "redo commits", "restructure commits", "squash by component", or "organize commits" -- Preparing a branch for PR review with clean commit history -- Branch has many small/WIP commits that should be consolidated - -**Covers:** - -- Component-based commit grouping (API, Vendor, CLI, HO, CPO, E2E, Docs) -- Soft reset and re-staging workflow -- Conventional commit messages with correct type/scope per component -- Edge cases for file categorization (support/, testdata/, API tests) - ### Fix HyperShift Repo Robot PR **Location:** `.claude/commands/fix-hypershift-repo-robot-pr.md` diff --git a/.claude/skills/restructure-commits/SKILL.md b/.claude/skills/restructure-commits/SKILL.md index 93543b6567f7..0bcfc6a720f5 100644 --- a/.claude/skills/restructure-commits/SKILL.md +++ b/.claude/skills/restructure-commits/SKILL.md @@ -3,125 +3,5 @@ name: restructure-commits description: Restructure branch commits into logical component-based commits for HyperShift PRs --- -# Restructure Commits by Component - -Reorganize all commits on a feature branch into logical, component-based commits that match HyperShift's architecture. - -## When to Use - -- User asks to "redo commits", "restructure commits", "squash by component", or "organize commits" -- Preparing a branch for PR review with clean commit history -- Branch has many small/WIP commits that should be consolidated - -## Component Categories and File Mapping - -Commits are created in this order. Each commit groups files by architectural boundary. - -| Order | Component | Scope | File Patterns | -|-------|-----------|-------|---------------| -| 1 | API | `api` | `api/` (types, deepcopy, CRD manifests, go.mod) — **excluding** `*_test.go` | -| 2 | Vendor | `api` | `vendor/`, `client/`, `cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/` | -| 3 | CLI | `cli` | `cmd/cluster/`, `cmd/install/`, `cmd/nodepool/`, `product-cli/` (source, tests, testdata) | -| 4 | HO | `hypershift-operator` | `hypershift-operator/`, `support/`, `karpenter-operator/`, `kubevirtexternalinfra/`, `pkg/`, `manifests/`, `shared-ingress/`, `sharedingress-config-generator/` | -| 5 | CPO | `control-plane-operator` | `control-plane-operator/`, `control-plane-pki-operator/`, `availability-prober/`, `dnsresolver/`, `etcd-backup/`, `etcd-defrag/`, `etcd-recovery/`, `ignition-server/`, `kas-bootstrap/`, `konnectivity-https-proxy/`, `konnectivity-socks5-proxy/`, `kubernetes-default-proxy/`, `sync-fg-configmap/`, `sync-global-pullsecret/`, `token-minter/` | -| 6 | E2E | `e2e` | `test/`, `api/**/*_test.go` | -| 7 | Docs | `docs` | `docs/`, `examples/` | - -**Excluded from restructuring:** `bin/` (build output), `hack/`, `contrib/`, `hypershift-ci-python/`, `self-managed-azure-ci-setup/`, and `.claude*/` are build tooling, CI helpers, or dev config. If changed, include them in the most relevant commit based on their purpose. When ambiguous, prefer HO. - -## Procedure - -### 1. Identify merge base and changed files - -```bash -BASE_BRANCH=$(gh pr view --json baseRefName -q .baseRefName 2>/dev/null || echo "main") -MERGE_BASE=$(git merge-base ${BASE_BRANCH} HEAD) -git log --oneline ${MERGE_BASE}..HEAD # review existing commits -git diff ${MERGE_BASE}..HEAD --name-only | sort # all changed files -``` - -### 2. Reset to merge base (keep changes) - -```bash -git reset --soft ${MERGE_BASE} # keep everything staged -git reset HEAD # unstage everything -``` - -### 3. Stage and commit each component group - -For each component (in order), stage matching files and commit: - -```bash -# Example: API commit (exclude test files — they belong in E2E) -git add api/ && git reset HEAD 'api/**/*_test.go' 2>/dev/null || true -git commit # use conventional commit format - -# Example: Vendor commit -git add vendor/ client/ \ - "cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/" -git commit - -# ... repeat for CLI, HO, CPO, E2E, Docs -``` - -### 4. Verify and force push - -```bash -git status # must be clean -git log --oneline ${BASE_BRANCH}..HEAD # verify commit structure -git push --force-with-lease # requires user confirmation -``` - -## Commit Message Conventions - -**Always invoke the git-commit-format skill first** for the full formatting rules (line length, footer, Co-Authored-By, etc.). This section provides the component-specific type and scope, plus guidance on writing the subject and body. - -### Writing the subject - -1. Look at the actual changes in the component to determine what was done -2. Pick the type and scope from the table below -3. Write a concise subject that summarizes the *purpose* of the changes, not just "update files" -4. Use imperative mood: "add", "update", "remove" — not "added", "adds", "adding" - -| Component | Type(Scope) | Example Subject | -|-----------|-------------|-----------------| -| API | `feat(api):` | `feat(api): add FooBar CRD and platform config` | -| Vendor | `chore(api):` | `chore(api): regenerate CRDs, clients, deepcopy, and vendor` | -| CLI | `feat(cli):` | `feat(cli): add --foo-bar flags for cluster creation` | -| HO | `feat(hypershift-operator):` | `feat(hypershift-operator): add FooBar controller` | -| CPO | `feat(control-plane-operator):` | `feat(control-plane-operator): add FooBar controllers` | -| E2E | `test(e2e):` | `test(e2e): add FooBar e2e and validation tests` | -| Docs | `docs:` | `docs: add FooBar documentation and architecture reference` | - -### Writing the body - -Review the staged changes and write a body that describes *what* was added or changed. Use bullet points when there are multiple distinct changes. The body should give a reviewer enough context to understand the commit without reading every file. Each line must be under 140 characters (gitlint enforced). - -### Notes - -- Vendor commit is always `chore(api):` since it's regenerated output from the API commit -- E2E commit is always `test(e2e):` regardless of what's being tested -- Docs commit has no scope parentheses — just `docs:` - -## Edge Cases - -- **Empty component:** Skip the commit if no files match that component. -- **Support packages:** `support/` goes with HO (commit 4), not CPO. -- **Control plane sidecars:** `control-plane-pki-operator/`, `availability-prober/`, `dnsresolver/`, `etcd-*`, `ignition-server/`, `kas-bootstrap/`, `konnectivity-*`, `kubernetes-default-proxy/`, `sync-fg-configmap/`, `sync-global-pullsecret/`, `token-minter/` all go with CPO (commit 5) since they are control plane components managed by CPO. -- **Shared ingress:** `shared-ingress/` and `sharedingress-config-generator/` go with HO (commit 4), not CPO, since they are part of the hypershift-operator. -- **Karpenter operator:** `karpenter-operator/` goes with HO (commit 4) since it is managed by the hypershift-operator. -- **Shared test fixtures in CPO:** `control-plane-operator/**/testdata/` stays with CPO. -- **Generated CRD install manifests:** `cmd/install/assets/hypershift-operator/zz_generated.crd-manifests/` goes with Vendor (commit 2), not CLI. -- **API go.mod:** `api/go.mod` goes with API (commit 1), not Vendor. -- **API test files:** `api/**/*_test.go` (UX API validation tests) go with E2E (commit 6), not API. Stage API non-test files first, then include API test files when staging E2E. -- **Documentation files:** `docs/` and `examples/` go with Docs (commit 7), not Vendor. This includes mkdocs config, how-to guides, architecture references, and aggregated docs. The `docs/content/reference/api.md` (generated API reference) also goes here. -- **Build/CI tooling:** `hack/`, `contrib/`, `hypershift-ci-python/`, `self-managed-azure-ci-setup/` are not mapped to a default component. Include in the most relevant commit based on purpose, or HO when ambiguous. - -## Quick Checklist - -- [ ] Determined base branch from PR (defaults to `main`) -- [ ] Reviewed all changed files before starting -- [ ] Reset with `--soft` (no data loss) -- [ ] Committed in correct order: API, Vendor, CLI, HO, CPO, E2E, Docs -- [ ] Working tree is clean after all commits -- [ ] Confirmed force push with user before executing +Read and follow the canonical procedure in `../../../skills/restructure-commits/SKILL.md` (repository path +`skills/restructure-commits/SKILL.md`) whenever restructuring, squashing, or reorganizing branch commits. diff --git a/.cursor/rules/restructure-commits.mdc b/.cursor/rules/restructure-commits.mdc new file mode 100644 index 000000000000..92daadd91669 --- /dev/null +++ b/.cursor/rules/restructure-commits.mdc @@ -0,0 +1,6 @@ +--- +description: restructuring, squashing, or reorganizing branch commits into component-based commits +alwaysApply: false +--- + +Read and follow the canonical procedure in `../../skills/restructure-commits/SKILL.md` whenever restructuring commits. diff --git a/.github/workflows/test-commit-layout-reusable.yaml b/.github/workflows/test-commit-layout-reusable.yaml new file mode 100644 index 000000000000..46b1ee2710e2 --- /dev/null +++ b/.github/workflows/test-commit-layout-reusable.yaml @@ -0,0 +1,18 @@ +name: Unit Tests (Restructure Commits) (Reusable) + +on: + workflow_call: + +permissions: + contents: read + +jobs: + test-commit-layout: + name: Restructure Commits + runs-on: arc-runner-set + timeout-minutes: 10 + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + with: + persist-credentials: false + - run: make test-commit-layout diff --git a/.github/workflows/test-commit-layout.yaml b/.github/workflows/test-commit-layout.yaml new file mode 100644 index 000000000000..7790c54c429e --- /dev/null +++ b/.github/workflows/test-commit-layout.yaml @@ -0,0 +1,17 @@ +name: Unit Tests (Restructure Commits) + +on: + pull_request: + branches: + - main + paths: + - 'skills/restructure-commits/**' + - 'Makefile' + - '.github/workflows/test-commit-layout.yaml' + - '.github/workflows/test-commit-layout-reusable.yaml' + +jobs: + test-commit-layout: + uses: openshift/hypershift/.github/workflows/test-commit-layout-reusable.yaml@main + permissions: + contents: read diff --git a/AGENTS.md b/AGENTS.md index e96000914ffc..4d7629d00db6 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,6 +22,7 @@ Project documentation is published via MkDocs. The site structure and navigation | **E2E v2 test framework** | [test/e2e/v2/AGENTS.md](test/e2e/v2/AGENTS.md) | | **Envtest (CEL validation tests)** | [test/envtest/README.md](test/envtest/README.md) — YAML-driven, runs across k8s 1.31–1.37, supports feature gate filtering | | **CEL over webhooks** | [.claude/rules/webhook-validation.md](.claude/rules/webhook-validation.md) | +| **Restructuring commits** | [skills/restructure-commits/SKILL.md](skills/restructure-commits/SKILL.md) — component-based commit layout; verify with `make verify-commit-layout` | | **Code formatting** | [DEVELOPMENT.md](DEVELOPMENT.md) — code quality commands and conventions | | **Unit test conventions** | [TESTING.md](TESTING.md) — naming, placement, and creation requirements | | **Design invariants** | [docs/content/reference/goals-and-design-invariants.md](docs/content/reference/goals-and-design-invariants.md) | diff --git a/DEVELOPMENT.md b/DEVELOPMENT.md index 1549ee84a0b6..e1a7cfdc3da0 100644 --- a/DEVELOPMENT.md +++ b/DEVELOPMENT.md @@ -292,7 +292,7 @@ Use the `git-commit-format` skill for full details and examples. ### Restructuring Commits Before PR Submission -Before creating a PR or after addressing review comments, use the `restructure-commits` skill to reorganize all branch commits into logical, component-based commits. This ensures every PR has a clean, reviewable commit history grouped by architectural boundary. +Before creating a PR or after addressing review comments, follow [`skills/restructure-commits/SKILL.md`](skills/restructure-commits/SKILL.md) to reorganize all branch commits into logical, component-based commits. Any agent or human can follow it; its ownership rules are generated into `ownership.json` by `make update`, and `make verify-commit-layout BASE=` must pass before pushing. ## Pull Requests @@ -300,7 +300,7 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for the full contribution guidelines. Key ### Before Creating a PR -1. Use the `restructure-commits` skill to organize commits by component (see [Restructuring Commits](#restructuring-commits-before-pr-submission) above) +1. Follow the restructure-commits procedure to organize commits by component (see [Restructuring Commits](#restructuring-commits-before-pr-submission) above) 2. Complete the proportionate validation and report the evidence described in [Pre-PR Gate](#pre-pr-gate). ### PR Title @@ -319,7 +319,7 @@ Follow the template in `.github/PULL_REQUEST_TEMPLATE.md`. ### After Review Comments -After addressing review feedback, use the `restructure-commits` skill again to reorganize commits before force-pushing. This keeps the commit history clean for subsequent review rounds. +After addressing review feedback, follow the restructure-commits procedure again to reorganize commits before force-pushing. This keeps the commit history clean for subsequent review rounds. ## Code Conventions diff --git a/Makefile b/Makefile index a066fa98a2fc..bab7198c4b66 100644 --- a/Makefile +++ b/Makefile @@ -97,6 +97,7 @@ CAPI_PROVIDERS := agent aws azure gcp ibmcloud kubevirt openstack .PHONY: update update: api-deps workspace-sync capi-sync deps api api-docs clients docs-aggregate + $(MAKE) ownership-rules GOLANGCI_LINT := $(abspath $(TOOLS_BIN_DIR)/golangci-lint) $(GOLANGCI_LINT): $(TOOLS_DIR)/go.mod # Build golangci-lint from tools folder. @@ -773,6 +774,25 @@ verify-tekton-pipeline-pairs: $(PYTHON_VENV_STAMP) ## Verify paired Tekton Pipel PYTHONPATH=$(PYTHON_VENV) python3 hack/verify-tekton-pipeline-pairs.py; \ fi +COMMIT_LAYOUT := skills/restructure-commits/commit_layout.py + +.PHONY: verify-commit-layout +verify-commit-layout: ## Verify component commit layout of BASE..HEAD_REF (restructure-commits gate). Set BASE; optional HEAD_REF, EXPECT_TREE. + @test -n "$(BASE)" || { echo "BASE must be set to the merge base with the PR target branch" >&2; exit 2; } + @python3 $(COMMIT_LAYOUT) verify --base "$(BASE)" --head "$(or $(HEAD_REF),HEAD)" $(if $(EXPECT_TREE),--expect-tree "$(EXPECT_TREE)") + +.PHONY: ownership-rules +ownership-rules: ## Regenerate the restructure-commits ownership.json and SKILL.md table from repository facts. + python3 skills/restructure-commits/generate_ownership.py + +.PHONY: test-commit-layout +test-commit-layout: $(PYTHON_VENV_STAMP) ## Test the restructure-commits layout tool and ownership generator. + @if [ -x $(PYTHON_VENV)/bin/python3 ]; then \ + $(PYTHON_VENV)/bin/python3 -m pytest -q -p no:cacheprovider skills/restructure-commits; \ + else \ + PYTHONPATH=$(PYTHON_VENV) python3 -m pytest -q -p no:cacheprovider skills/restructure-commits; \ + fi + .PHONY: verify-codespell verify-codespell: codespell ## Verify codespell. @$(CODESPELL) --count --ignore-words=./.codespellignore --skip="./hack/tools/bin/codespell_dist,./docs/site/*,./vendor/*,./api/vendor/*,./hack/tools/vendor/*,./api/hypershift/v1alpha1/*,./support/thirdparty/*,./docs/content/reference/*,./hack/tools/bin/*,./cmd/install/assets/*,./go.sum,./api/go.sum,./hack/workspace/go.work.sum,./api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests,./hack/tools/go.mod,./hack/tools/go.sum,./karpenter-operator/controllers/karpenter/assets/*.yaml,./dev/*,./pkg/capi/*,./hack/capi-vendor/*" diff --git a/docs/content/how-to/ci/github-actions.md b/docs/content/how-to/ci/github-actions.md index 7a3b8d3e436c..156df798735f 100644 --- a/docs/content/how-to/ci/github-actions.md +++ b/docs/content/how-to/ci/github-actions.md @@ -66,6 +66,7 @@ Pull request callers resolve their reusable workflows from `main`, while post-me | `cpo-container-sync.yaml` | `cpo-container-sync-reusable.yaml` | Validate CPO container image references are in sync | | `dependabot-commit-fix.yaml` | `dependabot-commit-fix-reusable.yaml` | Rewrite dependabot commit messages to pass gitlint | | `gocacheprog-test.yaml` | `gocacheprog-test-reusable.yaml` | Tests for the `contrib/ci/gocacheprog` build cache tool (path-filtered) | +| `test-commit-layout.yaml` | `test-commit-layout-reusable.yaml` | Tests for the `skills/restructure-commits` layout tool and ownership generator (path-filtered) | | `validate-cpo-overrides.yaml` | — | Validate CPO override images contain the PRs claimed in the PR description | !!! note diff --git a/docs/content/reference/aggregated-docs.md b/docs/content/reference/aggregated-docs.md index 841976aa9876..b3a0017f5d7e 100644 --- a/docs/content/reference/aggregated-docs.md +++ b/docs/content/reference/aggregated-docs.md @@ -14408,6 +14408,7 @@ Pull request callers resolve their reusable workflows from `main`, while post-me | `cpo-container-sync.yaml` | `cpo-container-sync-reusable.yaml` | Validate CPO container image references are in sync | | `dependabot-commit-fix.yaml` | `dependabot-commit-fix-reusable.yaml` | Rewrite dependabot commit messages to pass gitlint | | `gocacheprog-test.yaml` | `gocacheprog-test-reusable.yaml` | Tests for the `contrib/ci/gocacheprog` build cache tool (path-filtered) | +| `test-commit-layout.yaml` | `test-commit-layout-reusable.yaml` | Tests for the `skills/restructure-commits` layout tool and ownership generator (path-filtered) | | `validate-cpo-overrides.yaml` | — | Validate CPO override images contain the PRs claimed in the PR description | !!! note diff --git a/skills/restructure-commits/SKILL.md b/skills/restructure-commits/SKILL.md new file mode 100644 index 000000000000..bc7c3061395e --- /dev/null +++ b/skills/restructure-commits/SKILL.md @@ -0,0 +1,170 @@ +--- +name: restructure-commits +description: Restructure branch commits into logical component-based commits for HyperShift PRs +--- + +# Restructure Commits by Component + +Rewrite a feature branch into one commit per architectural group (API, Dependencies, Vendor, CLI, HO, CPO, E2E, +Docs, Tooling) without changing the branch's final file content. + +This is the canonical, model-neutral procedure. Any coding agent or human can follow it with `git`, `python3` +(standard library only), `make`, and optionally `gh`. All paths are relative to the repository root. + +## When to Use + +- The user asks to "redo", "restructure", "squash by component", or "organize" commits +- Preparing a branch for PR review, or cleaning history after addressing review comments + +## Ownership + +`ownership.json` maps every path to a group; `commit_layout.py` applies it. Both the file and the table below are +generated by `generate_ownership.py` (`make ownership-rules`, also run by `make update`); never edit them by hand. +Commit groups in this order and skip groups with no changes: + + +| Order | Group | Commit prefix | Allowed `` | Owns | +|-------|-------|---------------|------------------|------| +| 1 | API | `(api)` | `feat`, `fix`, `build`, `refactor` | API types, markers, and the api/ module definition (api/go.mod and api/go.sum stay together) | +| 2 | Dependencies | `build(deps)` | fixed | Root, tools, and workspace module definitions; never mixed with generated vendor trees | +| 3 | Vendor | `chore(api)` | fixed | Generated output: vendor trees, clients, zz_generated files, generated CRD manifests, and other files marked as generated | +| 4 | CLI | `(cli)` | `feat`, `fix`, `refactor`, `test` | hypershift and hcp CLIs, with their tests, testdata, and fixtures | +| 5 | HO | `(hypershift-operator)` | `feat`, `fix`, `refactor`, `perf`, `test` | hypershift-operator binaries and Go code shared by binaries of more than one group | +| 6 | CPO | `(control-plane-operator)` | `feat`, `fix`, `refactor`, `perf`, `test` | control-plane-operator binaries and the Go code only they use, such as control plane sidecars | +| 7 | E2E | `test(e2e)` | fixed | E2E, integration, and envtest suites, including API validation tests | +| 8 | Docs | `docs` | fixed | User and contributor documentation and agent guidance files | +| 9 | Tooling | `()` | `build`, `ci`, `chore`, `docs` | Everything else: build system, CI configuration, scripts, container builds, helper modules, AI agent tooling | + + +- **Derived, not curated.** Each binary built by `make build` belongs to the group in the generator's policy, and + other top-level Go directories follow the binaries that import them (several groups: HO; none: Tooling). Files + marked as generated, Makefile output directories, and vendor trees go to Vendor; hand-written files inside them + keep their component's group. +- **First match wins.** Specific exceptions are listed before broad directories; `ownership.json` lists the rules in + precedence order with the reason for each. +- **Modules vs generated trees:** `api/go.mod` and `api/go.sum` stay in API; root, tools, and workspace module files + go in Dependencies; every vendor tree goes in Vendor. A commit must never mix module definitions with vendor output. +- **Tests and fixtures:** component tests, `testdata/`, and `zz_fixture_*` files stay with their owning component. + Only `test/`, `api/**/*_test.go`, and CRD envtest suites go to E2E. +- **No silent defaults:** an unclassified path blocks the rewrite. Run `make ownership-rules`, which gives every + path in the checkout a group, and commit the result before the snapshot. If a derived group is wrong, agree the + right one with the user and change the generator's policy (see Maintenance). +- Several consecutive commits in the same group are allowed; groups must never interleave or appear out of order. + +## Procedure + +```bash +TOOL=skills/restructure-commits/commit_layout.py +``` + +### 1. Preflight (read-only except for the backup branch) + +1. **Resolve the real target.** Determine the PR base repository and branch, for example from + `gh pr view --json url,baseRefName`; ask the user when there is no PR. Pick the remote whose URL points at that + base repository and the remote that hosts the PR branch. Do not assume remote names, do not assume `main` + (release branches are valid targets), and do not rebase unless the user asks for it. + + ```bash + BRANCH=$(git symbolic-ref --short HEAD) + TARGET_REMOTE= + TARGET_BRANCH= + PUSH_REMOTE= + git fetch "$TARGET_REMOTE" "refs/heads/$TARGET_BRANCH" + MERGE_BASE=$(git merge-base FETCH_HEAD HEAD) + PUSHED_SHA=$(git ls-remote "$PUSH_REMOTE" "refs/heads/$BRANCH" | cut -f1) # empty if never pushed + if [ -n "$PUSHED_SHA" ]; then + git fetch "$PUSH_REMOTE" "refs/heads/$BRANCH" + git merge-base --is-ancestor "$PUSHED_SHA" HEAD || echo "STOP: remote $BRANCH has commits not in HEAD" + fi + ``` + + If the remote branch has commits that are not in local `HEAD` (review suggestions, "Update branch", a + collaborator's push), stop and ask the user to integrate them first; the rewrite would otherwise discard them. + +2. **Finish background work.** Wait for running generators, formatters, `make update`, or tests that write files. +3. **Handle existing work explicitly.** If `git status --porcelain --untracked-files=all` is not empty, stop and ask + whether to commit it to the branch first, stash it with + `git stash push --include-untracked -m "restructure-commits: preserved work"` (restore it at the end), or abort. + Never use `git reset --hard`, `git checkout -- .`, `git clean`, or drop stashes or branches. +4. **Record the snapshot and create a backup branch.** Report these values to the user. + + ```bash + ORIG_HEAD_SHA=$(git rev-parse HEAD) + ORIG_TREE=$(git rev-parse "HEAD^{tree}") + BACKUP="backup/restructure/$BRANCH-$(date +%Y%m%d%H%M%S)" + git branch "$BACKUP" "$ORIG_HEAD_SHA" + ``` + +5. **Show the complete plan.** Resolve every unclassified path, then present the file-to-commit plan with the + proposed commit subjects and get the user's approval before rewriting. + + ```bash + python3 "$TOOL" plan --base "$MERGE_BASE" --head "$ORIG_HEAD_SHA" + ``` + +### 2. Rewrite history (lossless) + +```bash +git reset --soft "$MERGE_BASE" && git reset -q # index = merge base; working tree still equals $ORIG_TREE +``` + +For each non-empty group, in table order: + +```bash +python3 "$TOOL" stage --base "$MERGE_BASE" --head "$ORIG_HEAD_SHA" --group +git commit -s # message rules below +``` + +`stage` copies exactly that group's paths (including deletions and paths with spaces) from the snapshot into the +index and fails if anything else is staged. + +### 3. Verify (required before declaring completion) + +```bash +python3 "$TOOL" verify --base "$MERGE_BASE" --head HEAD --expect-tree "$ORIG_TREE" +make run-gitlint PULL_BASE_SHA="$MERGE_BASE" PULL_PULL_SHA=HEAD # message syntax only, not file grouping +git status --porcelain --untracked-files=all # must be empty +``` + +`make verify-commit-layout BASE="$MERGE_BASE" EXPECT_TREE="$ORIG_TREE"` is equivalent to the first command. Without +`make`, run `gitlint --commits "$MERGE_BASE..HEAD"` instead. The layout check verifies that every commit in the range +touches a single group, that groups are in order, and that no path is unclassified, and confirms the final tree +equals the snapshot. If it fails, do not push: fix the commits and re-run, or show +`git diff "$ORIG_TREE" HEAD` and, with the user's agreement, return to the snapshot with `git reset --keep "$BACKUP"`. +Restore any stash from preflight with `git stash pop`. + +### 4. Push (only after explicit user confirmation) + +```bash +git push --force-with-lease="refs/heads/$BRANCH:$PUSHED_SHA" "$PUSH_REMOTE" "HEAD:refs/heads/$BRANCH" +``` + +The lease protects against remote updates made after preflight (earlier ones were checked in preflight). If it is +rejected, stop and ask. Keep `$BACKUP` until the user agrees to +delete it. + +## Commit Messages + +Read and follow `skills/git-commit-format/SKILL.md` directly (harnesses with native skill support may load it as +the `git-commit-format` skill). Validate with gitlint as shown above. + +- Use the group's prefix from the table. Where it contains ``, pick the allowed type that describes the change + (`feat` new behavior, `fix` bug fix, `build` dependency or build change, `refactor` restructuring or removal, `perf` + performance, `test` test or fixture-only change); for Tooling also choose a fitting scope such as `ci(envtest)`. + Fixed prefixes are used as-is; Docs is `docs:` without a scope. +- Subject: imperative mood, describes the purpose (for example `build(deps): bump Kubernetes dependencies to 1.37`). +- Body: what changed and why, bullets for distinct changes; title at most 120 and body lines at most 140 characters. + +## Maintenance + +- **Change ownership:** edit the POLICY section of `generate_ownership.py` (groups, binary groups, conventions), + then run `make ownership-rules` and `make test-commit-layout`. Because `make update` regenerates the rules, + `make verify` fails when a PR leaves `ownership.json` or this table stale. +- **Practice or demonstrate:** `python3 skills/restructure-commits/sample_branch.py ` builds a + repository whose branch reproduces the PR #9568 module/vendor mistake; follow this procedure in `/work`. +- **Enforcement scope:** the layout check is a required gate of this workflow only; it is not enforced repository-wide + by pre-push hooks or CI. Gitlint remains the repository-wide message check. The tool's own tests run in the + path-filtered `Unit Tests (Restructure Commits)` GitHub workflow. +- **Adapters:** harness-specific entry points (`.claude/skills/restructure-commits/SKILL.md`, + `.cursor/rules/restructure-commits.mdc`) only point here. Replace personal copies (such as a user-installed + `restructure-hypershift-commits` skill) with a pointer to this file instead of maintaining separate rules. diff --git a/skills/restructure-commits/commit_layout.py b/skills/restructure-commits/commit_layout.py new file mode 100644 index 000000000000..1f0d6125e1fa --- /dev/null +++ b/skills/restructure-commits/commit_layout.py @@ -0,0 +1,305 @@ +#!/usr/bin/env python3 +"""Plan, stage, and verify HyperShift component-based commit layouts. + +The ownership rules live in ownership.json next to this script; generate_ownership.py +derives them from the repository (make update), so never edit them by hand. +Only the Python 3 standard library and the git CLI are required. +""" + +from __future__ import annotations + +import argparse +import fnmatch +import json +import os +import subprocess +import sys +from dataclasses import dataclass +from pathlib import Path + +SCRIPT_DIR = Path(__file__).resolve().parent +RULES_FILE = SCRIPT_DIR / "ownership.json" + +EXIT_OK = 0 +EXIT_VIOLATION = 1 +EXIT_USAGE = 2 + + +class UsageError(Exception): + pass + + +@dataclass(frozen=True) +class Group: + order: int + name: str + prefix: str + + +@dataclass(frozen=True) +class Rule: + pattern: str + group: str + + +@dataclass(frozen=True) +class Change: + status: str + path: str + + +def match_path(pattern: str, path: str) -> bool: + """Match a root-anchored glob where '**' spans zero or more whole segments.""" + return _match_segments(pattern.split("/"), path.split("/")) + + +def _match_segments(pattern: list[str], path: list[str]) -> bool: + if not pattern: + return not path + head, rest = pattern[0], pattern[1:] + if head == "**": + return any(_match_segments(rest, path[i:]) for i in range(len(path) + 1)) + if not path: + return False + return fnmatch.fnmatchcase(path[0], head) and _match_segments(rest, path[1:]) + + +class Ownership: + def __init__(self, groups: list[Group], rules: list[Rule]): + self.groups = groups + self.rules = rules + self._by_name = {g.name: g for g in groups} + for rule in rules: + if rule.group not in self._by_name: + raise UsageError(f"rule {rule.pattern!r} references unknown group {rule.group!r}") + + @classmethod + def load(cls, path: Path = RULES_FILE) -> "Ownership": + data = json.loads(Path(path).read_text()) + groups = [Group(order=i + 1, name=g["name"], prefix=g["prefix"]) for i, g in enumerate(data["groups"])] + return cls(groups, [Rule(r["pattern"], r["group"]) for r in data["rules"]]) + + def group(self, name: str) -> Group: + try: + return self._by_name[name] + except KeyError: + raise UsageError(f"unknown group {name!r}; expected one of: {', '.join(self._by_name)}") from None + + def classify(self, path: str) -> Group | None: + """Return the group of the first matching rule, or None when no rule matches.""" + for rule in self.rules: + if match_path(rule.pattern, path): + return self._by_name[rule.group] + return None + + +def git(*args: str, stdin: bytes | None = None, env: dict[str, str] | None = None) -> bytes: + proc = subprocess.run( + ["git", "-c", "core.quotepath=off", *args], + input=stdin, + env=env, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + ) + if proc.returncode != 0: + raise UsageError(f"git {' '.join(args)} failed: {proc.stderr.decode(errors='replace').strip()}") + return proc.stdout + + +def resolve(rev: str, kind: str = "commit") -> str: + return git("rev-parse", "--verify", "--end-of-options", f"{rev}^{{{kind}}}").decode().strip() + + +def decode_paths(raw: bytes) -> list[str]: + return [p.decode("utf-8", errors="surrogateescape") for p in raw.split(b"\0") if p] + + +def diff_changes(old: str, new: str) -> list[Change]: + """List changed paths between two commits. Renames are reported as delete plus add.""" + fields = decode_paths(git("diff", "--no-renames", "--no-ext-diff", "--name-status", "-z", old, new, "--")) + return [Change(fields[i], fields[i + 1]) for i in range(0, len(fields), 2)] + + +def build_plan(ownership: Ownership, changes: list[Change]) -> tuple[dict[str, list[Change]], list[Change]]: + by_group: dict[str, list[Change]] = {g.name: [] for g in ownership.groups} + unclassified = [] + for change in sorted(changes, key=lambda c: c.path): + group = ownership.classify(change.path) + if group is None: + unclassified.append(change) + else: + by_group[group.name].append(change) + return by_group, unclassified + + +def resolve_range(base: str, head: str) -> tuple[str, str]: + base_sha, head_sha = resolve(base), resolve(head) + try: + git("merge-base", "--is-ancestor", base_sha, head_sha) + except UsageError: + raise UsageError(f"base {base} is not an ancestor of head {head}; pass the merge base with the PR target") from None + return base_sha, head_sha + + +def load_plan(args: argparse.Namespace) -> tuple[Ownership, str, str, dict[str, list[Change]], list[Change]]: + ownership = Ownership.load() + base, head = resolve_range(args.base, args.head) + by_group, unclassified = build_plan(ownership, diff_changes(base, head)) + return ownership, base, head, by_group, unclassified + + +def cmd_plan(args: argparse.Namespace) -> int: + ownership, base, head, by_group, unclassified = load_plan(args) + total = sum(len(v) for v in by_group.values()) + len(unclassified) + print(f"Commit layout plan for {base[:12]}..{head[:12]} ({total} changed paths)") + commit_no = 0 + empty = [] + for g in ownership.groups: + changes = by_group[g.name] + if not changes: + empty.append(g.name) + continue + commit_no += 1 + print(f"\nCommit {commit_no}: {g.name} ({g.prefix}) - {len(changes)} path(s)") + for c in changes: + print(f" {c.status:<2} {c.path}") + if empty: + print(f"\nSkipped empty groups: {', '.join(empty)}") + if unclassified: + print("\nUNCLASSIFIED paths - add a rule to ownership.json before rewriting history:") + for c in unclassified: + print(f" {c.status:<2} {c.path}") + return EXIT_VIOLATION + return EXIT_OK + + +def cmd_stage(args: argparse.Namespace) -> int: + """Stage exactly one group's paths with their content from --head (the recorded original).""" + ownership, _, head, by_group, unclassified = load_plan(args) + target = ownership.group(args.group) + if unclassified: + print(f"error: {len(unclassified)} unclassified path(s); run plan and resolve them first", file=sys.stderr) + return EXIT_VIOLATION + if git("diff", "--cached", "--name-only", "-z"): + print("error: the index already has staged changes; commit or unstage them before staging a group", file=sys.stderr) + return EXIT_VIOLATION + paths = [c.path for c in by_group[target.name]] + if not paths: + print(f"{target.name}: no changes; skip this group") + return EXIT_OK + + git( + "restore", f"--source={head}", "--staged", "--pathspec-from-file=-", "--pathspec-file-nul", + stdin="".join(p + "\0" for p in paths).encode("utf-8", errors="surrogateescape"), + env=dict(os.environ, GIT_LITERAL_PATHSPECS="1"), + ) + + staged = set(decode_paths(git("diff", "--cached", "--no-renames", "--name-only", "-z"))) + expected = set(paths) + if staged != expected: + print(f"error: staged paths do not match the {target.name} plan", file=sys.stderr) + for p in sorted(expected - staged): + print(f" missing: {p}", file=sys.stderr) + for p in sorted(staged - expected): + print(f" unexpected: {p}", file=sys.stderr) + return EXIT_VIOLATION + print(f"Staged {len(paths)} path(s) for {target.name}; commit with prefix {target.prefix}") + return EXIT_OK + + +def cmd_verify(args: argparse.Namespace) -> int: + ownership = Ownership.load() + base, head = resolve_range(args.base, args.head) + errors: list[str] = [] + + log = git("log", "--no-show-signature", "--reverse", "--topo-order", "--format=%H %P%x00%s", f"{base}..{head}").decode() + commits = [line.split("\0", 1) for line in log.split("\n") if line] + if not commits: + errors.append(f"no commits in {base[:12]}..{head[:12]}") + + last_group: Group | None = None + last_commit = "" + for shas, subject in commits: + sha, *parents = shas.split() + label = f"{sha[:12]} {subject!r}" + if len(parents) != 1: + errors.append(f"{label}: merge commits are not allowed in a restructured branch") + continue + changes = diff_changes(parents[0], sha) + if not changes: + errors.append(f"{label}: commit changes no files") + continue + by_group, unclassified = build_plan(ownership, changes) + present = [g for g in ownership.groups if by_group[g.name]] + for c in unclassified: + errors.append(f"{label}: unclassified path {c.path!r}") + if len(present) > 1: + detail = "; ".join(f"{g.name}: {', '.join(c.path for c in by_group[g.name][:5])}" + + (" ..." if len(by_group[g.name]) > 5 else "") for g in present) + names = {g.name for g in present} + hint = "" + if "Vendor" in names and names & {"API", "Dependencies"}: + hint = " (module definitions must not share a commit with generated vendor trees)" + errors.append(f"{label}: mixes groups {', '.join(g.name for g in present)}{hint}: {detail}") + if len(present) == 1: + group = present[0] + if last_group is not None and group.order < last_group.order: + errors.append( + f"{label}: {group.name} (order {group.order}) comes after {last_commit} " + f"{last_group.name} (order {last_group.order}); expected order " + f"{' -> '.join(g.name for g in ownership.groups)}" + ) + last_group, last_commit = group, sha[:12] + + if args.expect_tree: + expected, actual = resolve(args.expect_tree, "tree"), resolve(head, "tree") + if expected != actual: + changed = diff_changes(args.expect_tree, head) + listing = ", ".join(f"{c.status} {c.path}" for c in changed[:10]) + (" ..." if len(changed) > 10 else "") + errors.append(f"tree mismatch: head tree {actual[:12]} != expected {expected[:12]}: {listing}") + + if errors: + print(f"Commit layout verification FAILED for {base[:12]}..{head[:12]}:") + for e in errors: + print(f" - {e}") + return EXIT_VIOLATION + print(f"Commit layout OK: {len(commits)} commit(s) in {base[:12]}..{head[:12]}" + + (", tree matches expected snapshot" if args.expect_tree else "")) + return EXIT_OK + + +def build_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser(description=__doc__, formatter_class=argparse.RawDescriptionHelpFormatter) + sub = parser.add_subparsers(dest="command", required=True) + + def add_range(p: argparse.ArgumentParser) -> None: + p.add_argument("--base", required=True, help="merge base with the PR target branch") + p.add_argument("--head", default="HEAD", help="last commit of the range (default: HEAD)") + + p = sub.add_parser("plan", help="show the file-to-commit plan for base..head") + add_range(p) + p.set_defaults(func=cmd_plan) + + p = sub.add_parser("stage", help="stage one group's paths with content from --head") + add_range(p) + p.add_argument("--group", required=True) + p.set_defaults(func=cmd_stage) + + p = sub.add_parser("verify", help="verify file membership and group order of each commit in base..head") + add_range(p) + p.add_argument("--expect-tree", help="commit or tree that HEAD's tree must equal (the pre-rewrite snapshot)") + p.set_defaults(func=cmd_verify) + return parser + + +def main(argv: list[str] | None = None) -> int: + args = build_parser().parse_args(argv) + try: + return args.func(args) + except UsageError as e: + print(f"error: {e}", file=sys.stderr) + return EXIT_USAGE + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/restructure-commits/commit_layout_test.py b/skills/restructure-commits/commit_layout_test.py new file mode 100644 index 000000000000..cdf0a0009328 --- /dev/null +++ b/skills/restructure-commits/commit_layout_test.py @@ -0,0 +1,294 @@ +"""Regression tests for commit_layout.py using temporary Git repositories.""" + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +import commit_layout # noqa: E402 +import sample_branch # noqa: E402 +from sample_branch import commit, write # noqa: E402 + +SKILL_DIR = Path(__file__).resolve().parent + + +def git(repo: Path, *args: str) -> str: + return sample_branch.run(repo, "git", *args).strip() + + +def layout(repo: Path, *args: str) -> tuple[int, str, str]: + proc = subprocess.run( + [sys.executable, str(SKILL_DIR / "commit_layout.py"), *args], + cwd=repo, + stdout=subprocess.PIPE, + stderr=subprocess.PIPE, + text=True, + ) + return proc.returncode, proc.stdout, proc.stderr + + +def plan_paths(repo: Path, base: str, head: str, monkeypatch: pytest.MonkeyPatch) -> dict[str, list[tuple[str, str]]]: + monkeypatch.chdir(repo) + by_group, unclassified = commit_layout.build_plan(commit_layout.Ownership.load(), commit_layout.diff_changes(base, head)) + assert not unclassified + return {name: [(c.status, c.path) for c in changes] for name, changes in by_group.items() if changes} + + +@pytest.fixture(autouse=True) +def isolated_git_env(monkeypatch: pytest.MonkeyPatch) -> None: + """Keep git in tests on the temporary repositories even when run from a Git hook.""" + for key in [k for k in os.environ if k.startswith("GIT_")]: + monkeypatch.delenv(key) + + +def test_git_ignores_inherited_repository_overrides(repo: Path, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv("GIT_DIR", str(tmp_path / "caller.git")) + monkeypatch.setenv("GIT_INDEX_FILE", str(tmp_path / "caller.index")) + assert git(repo, "rev-parse", "--absolute-git-dir") == str((repo / ".git").resolve()) + + +@pytest.fixture +def repo(tmp_path: Path) -> Path: + sample_branch.init_repo(tmp_path) + commit(tmp_path, "chore: base", {"README.md": "base\n", "go.mod": "module x\n", "api/go.mod": "module x/api\n"}) + return tmp_path + + +@pytest.mark.parametrize( + "pattern,path,expected", + [ + ("api/**", "api/go.mod", True), + ("api/**/zz_generated*", "api/zz_generated.deepcopy.go", True), + ("api/**/zz_generated*", "api/hypershift/v1beta1/zz_generated.deepcopy.go", True), + ("*.md", "README.md", True), + ("*.md", "docs/README.md", False), + ("**/AGENTS.md", "AGENTS.md", True), + ("cmd/install/assets/crds/*.go", "cmd/install/assets/crds/sub/x.go", False), + (".*", ".gitignore", True), + (".*/**", ".github/workflows/a b.yaml", True), + ], +) +def test_match_path_semantics(pattern: str, path: str, expected: bool) -> None: + assert commit_layout.match_path(pattern, path) is expected + + +@pytest.mark.parametrize( + "path,group", + [ + ("go.mod", "Dependencies"), + ("go.sum", "Dependencies"), + ("hack/tools/go.mod", "Dependencies"), + ("hack/workspace/go.work", "Dependencies"), + ("api/go.mod", "API"), + ("api/go.sum", "API"), + ("api/.golangci.yml", "API"), + ("vendor/modules.txt", "Vendor"), + ("api/vendor/modules.txt", "Vendor"), + ("api/vendor/k8s.io/api/core/v1/x_test.go", "Vendor"), + ("hack/tools/vendor/modules.txt", "Vendor"), + ("client/clientset/clientset/clientset.go", "Vendor"), + ("api/hypershift/v1beta1/zz_generated.deepcopy.go", "Vendor"), + ("api/hypershift/v1beta1/zz_generated.featuregated-crd-manifests/hostedclusters/AAA.yaml", "Vendor"), + ("api/hypershift/v1beta1/hostedcluster_types_test.go", "E2E"), + ("api/AGENTS.md", "Docs"), + ("cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters.crd.yaml", "Vendor"), + ("cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/doc.go", "CLI"), + ("cmd/install/assets/crds/assets.go", "CLI"), + ("cmd/install/assets/crds/hypershift-operator/tests/nodepools.hypershift.openshift.io/a.yaml", "E2E"), + ("cmd/infra/aws/delegating_client.go", "Vendor"), + ("cmd/cluster/aws/testdata/zz_fixture_TestCreate.yaml", "CLI"), + ("karpenter-operator/controllers/karpenter/assets/karpenter.sh_nodepools.yaml", "Vendor"), + ("karpenter-operator/controllers/karpenter/assets/assets.go", "HO"), + ("karpenter-operator/controllers/karpenter/assets/tests/x/stable.testsuite.yaml", "E2E"), + ("shared-ingress/Containerfile", "Tooling"), + ("ignition-server/main.go", "HO"), + ("etcd-recovery/etcdrecovery.go", "HO"), + ("support/awsapi/ec2.go", "Vendor"), + ("sharedingress-config-generator/config.go", "HO"), + ("etcd-upload/uploader.go", "CPO"), + ("control-plane-operator/controllers/x/testdata/zz_fixture_TestX.yaml", "CPO"), + ("docs/content/reference/api.md", "Docs"), + ("DEVELOPMENT.md", "Docs"), + (".claude/skills/restructure-commits/SKILL.md", "Tooling"), + ("skills/restructure-commits/commit_layout.py", "Tooling"), + ("Makefile", "Tooling"), + (".github/workflows/gitlint.yaml", "Tooling"), + ("contrib/cleanroles/go.mod", "Tooling"), + ], +) +def test_classification_specific_rules_win(path: str, group: str) -> None: + ownership = commit_layout.Ownership.load() + assert ownership.classify(path).name == group + + +def test_unknown_path_is_not_defaulted() -> None: + assert commit_layout.Ownership.load().classify("brand-new-component/main.go") is None + + +def test_rules_must_reference_known_groups() -> None: + with pytest.raises(commit_layout.UsageError): + commit_layout.Ownership([], [commit_layout.Rule("x/**", "NotAGroup")]) + + +def test_verify_rejects_module_definitions_mixed_with_vendor(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + sha = commit(repo, "chore(api): regenerate", {"go.mod": "module x\n// bump\n", "vendor/modules.txt": "# bump\n"}) + code, out, _ = layout(repo, "verify", "--base", base) + assert code == 1 + assert sha[:12] in out + assert "Dependencies, Vendor" in out + assert "module definitions must not share a commit with generated vendor trees" in out + assert "go.mod" in out and "vendor/modules.txt" in out + + +def test_verify_accepts_api_modules_dependencies_and_vendor_in_separate_commits(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + commit(repo, "build(api): bump", {"api/go.mod": "module x/api\n// bump\n", "api/go.sum": "sum\n"}) + commit(repo, "build(deps): bump", {"go.mod": "module x\n// bump\n", "go.sum": "sum\n"}) + commit(repo, "chore(api): vendor", {"vendor/modules.txt": "# bump\n"}) + code, out, _ = layout(repo, "verify", "--base", base) + assert code == 0, out + + +def test_verify_rejects_root_and_api_modules_in_one_commit(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + commit(repo, "build: bump both", {"api/go.mod": "module x/api\n// again\n", "go.mod": "module x\n// again\n"}) + code, out, _ = layout(repo, "verify", "--base", base) + assert code == 1 + assert "mixes groups API, Dependencies" in out + + +def test_verify_rejects_out_of_order_groups(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + commit(repo, "chore(api): vendor", {"vendor/modules.txt": "x\n"}) + late = commit(repo, "feat(api): types", {"api/types.go": "package api\n"}) + code, out, _ = layout(repo, "verify", "--base", base) + assert code == 1 + assert late[:12] in out and "API (order 1) comes after" in out + + +def test_verify_allows_consecutive_commits_in_one_group(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + commit(repo, "feat(hypershift-operator): a", {"hypershift-operator/a.go": "package a\n"}) + commit(repo, "feat(hypershift-operator): b", {"support/b.go": "package b\n"}) + assert layout(repo, "verify", "--base", base)[0] == 0 + + +def test_verify_reports_unclassified_paths(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + sha = commit(repo, "feat: new thing", {"brand-new/main.go": "package main\n"}) + code, out, _ = layout(repo, "verify", "--base", base) + assert code == 1 + assert f"{sha[:12]}" in out and "unclassified path 'brand-new/main.go'" in out + + +def test_plan_blocks_on_unknown_paths_and_skips_empty_groups(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + commit(repo, "wip", {"api/types.go": "package api\n", "mystery/file.txt": "?\n"}) + code, out, _ = layout(repo, "plan", "--base", base) + assert code == 1 + assert "Commit 1: API ((api))" in out + assert "UNCLASSIFIED" in out and "mystery/file.txt" in out + assert "Skipped empty groups: Dependencies, Vendor, CLI, HO, CPO, E2E, Docs, Tooling" in out + + +def test_plan_accounts_for_renames_deletions_and_spaces_once(repo: Path, monkeypatch: pytest.MonkeyPatch) -> None: + commit(repo, "seed", {"docs/old name.md": "doc\n", "hypershift-operator/gone.go": "package x\n"}) + base = git(repo, "rev-parse", "HEAD") + head = commit(repo, "wip", {"cmd/new name.md": "doc\n"}, delete=("docs/old name.md", "hypershift-operator/gone.go")) + assert plan_paths(repo, base, head, monkeypatch) == { + "CLI": [("A", "cmd/new name.md")], + "HO": [("D", "hypershift-operator/gone.go")], + "Docs": [("D", "docs/old name.md")], + } + code, out, _ = layout(repo, "plan", "--base", base) + assert code == 0, out + assert "A cmd/new name.md" in out and "D docs/old name.md" in out + + +def test_verify_detects_tree_mismatch(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + original = commit(repo, "wip", {"hypershift-operator/a.go": "package a\n"}) + git(repo, "reset", "-q", "--hard", base) + commit(repo, "feat(hypershift-operator): a", {"hypershift-operator/a.go": "package a // changed\n"}) + code, out, _ = layout(repo, "verify", "--base", base, "--expect-tree", original) + assert code == 1 and "tree mismatch" in out and "hypershift-operator/a.go" in out + + +def test_verify_rejects_merge_commits(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + git(repo, "checkout", "-q", "-b", "side") + commit(repo, "feat(hypershift-operator): side", {"hypershift-operator/side.go": "package s\n"}) + git(repo, "checkout", "-q", "main") + commit(repo, "feat(hypershift-operator): main", {"hypershift-operator/main.go": "package m\n"}) + git(repo, "merge", "-q", "--no-edit", "side") + code, out, _ = layout(repo, "verify", "--base", base) + assert code == 1 and "merge commits are not allowed" in out + + +def test_stage_refuses_preexisting_staged_changes(repo: Path) -> None: + base = git(repo, "rev-parse", "HEAD") + original = commit(repo, "wip", {"hypershift-operator/a.go": "package a\n"}) + git(repo, "reset", "-q", base) + write(repo, {"unrelated.txt": "user work\n"}) + git(repo, "add", "unrelated.txt") + code, _, err = layout(repo, "stage", "--base", base, "--head", original, "--group", "HO") + assert code == 1 and "already has staged changes" in err + + +def test_sample_branch_reproduces_pr_9568_and_restructures_cleanly(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.chdir(tmp_path) + work = sample_branch.create_sample(Path("sample")) # relative, as on the command line + base = git(work, "merge-base", "origin/main", "HEAD") + original = git(work, "rev-parse", "HEAD") + + code, out, _ = layout(work, "verify", "--base", base) + assert code == 1 + assert "module definitions must not share a commit with generated vendor trees" in out + + plan = {name: sorted(p for _, p in changes) for name, changes in plan_paths(work, base, original, monkeypatch).items()} + assert plan == { + "API": ["api/go.mod", "api/go.sum"], + "Dependencies": ["go.mod", "go.sum"], + "Vendor": [ + "api/hypershift/v1beta1/zz_generated.deepcopy.go", + "api/vendor/k8s.io/api/core/v1/types.go", + "api/vendor/modules.txt", + "cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters.crd.yaml", + "vendor/k8s.io/api/core/v1/types.go", + "vendor/modules.txt", + ], + "HO": ["hypershift-operator/controllers/legacy/legacy.go"], + "CPO": [ + "control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/testdata/" + "zz_fixture_TestKubeScheduler.yaml" + ], + "E2E": [ + "cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/" + "k8s-1.37.testsuite.yaml", + "test/envtest/README.md", + ], + "Docs": ["AGENTS.md", "docs/content/how-to/k8s new guide.md", "docs/content/how-to/k8s old guide.md"], + "Tooling": [".github/workflows/envtest-kube-reusable.yaml", "Makefile"], + } + + _rewrite_all_groups(work, base, original) + code, out, _ = layout(work, "verify", "--base", base, "--expect-tree", original) + assert code == 0, out + assert git(work, "status", "--porcelain") == "" + + +def _rewrite_all_groups(repo: Path, base: str, original: str) -> None: + git(repo, "reset", "-q", "--soft", base) + git(repo, "reset", "-q") + for group in commit_layout.Ownership.load().groups: + code, out, err = layout(repo, "stage", "--base", base, "--head", original, "--group", group.name) + assert code == 0, err + if "no changes" not in out: + git(repo, "commit", "-q", "-m", group.name) diff --git a/skills/restructure-commits/generate_ownership.py b/skills/restructure-commits/generate_ownership.py new file mode 100644 index 000000000000..aaf5424afc5e --- /dev/null +++ b/skills/restructure-commits/generate_ownership.py @@ -0,0 +1,330 @@ +#!/usr/bin/env python3 +"""Generate ownership.json and the SKILL.md group table from facts in the repository. + +Run from the repository root; `make update` runs it. Only the POLICY section is written +by hand. Every path rule is derived from: + +- binaries: the Makefile `build` target and each binary's `-o ` recipe +- components: `go list -deps` of each binary +- module definitions and vendor trees: go.mod, go.work, and vendor/modules.txt locations +- generated files: Go "Code generated ... DO NOT EDIT." headers, the controller-gen CRD + annotation, and the directories that Makefile recipes write generated output to +- envtest suites: directories named tests that contain *.testsuite.yaml files +""" + +from __future__ import annotations + +import json +import os +import re +import subprocess +import sys +from collections import defaultdict +from dataclasses import dataclass, field +from pathlib import Path + +SCRIPT_DIR = Path(__file__).resolve().parent +sys.path.insert(0, str(SCRIPT_DIR)) + +from commit_layout import RULES_FILE, match_path # noqa: E402 + +SKILL_FILE = SCRIPT_DIR / "SKILL.md" +BEGIN_MARKER = "" +END_MARKER = "" +GO_MODULE = "github.com/openshift/hypershift/" + +# ---- POLICY (hand-written) ------------------------------------------------------------- + +GROUPS = [ + {"name": "API", "prefix": "(api)", "types": ["feat", "fix", "build", "refactor"], + "owns": "API types, markers, and the api/ module definition (api/go.mod and api/go.sum stay together)"}, + {"name": "Dependencies", "prefix": "build(deps)", "types": ["build"], + "owns": "Root, tools, and workspace module definitions; never mixed with generated vendor trees"}, + {"name": "Vendor", "prefix": "chore(api)", "types": ["chore"], + "owns": "Generated output: vendor trees, clients, zz_generated files, generated CRD manifests, and other files marked as generated"}, + {"name": "CLI", "prefix": "(cli)", "types": ["feat", "fix", "refactor", "test"], + "owns": "hypershift and hcp CLIs, with their tests, testdata, and fixtures"}, + {"name": "HO", "prefix": "(hypershift-operator)", "types": ["feat", "fix", "refactor", "perf", "test"], + "owns": "hypershift-operator binaries and Go code shared by binaries of more than one group"}, + {"name": "CPO", "prefix": "(control-plane-operator)", "types": ["feat", "fix", "refactor", "perf", "test"], + "owns": "control-plane-operator binaries and the Go code only they use, such as control plane sidecars"}, + {"name": "E2E", "prefix": "test(e2e)", "types": ["test"], + "owns": "E2E, integration, and envtest suites, including API validation tests"}, + {"name": "Docs", "prefix": "docs", "types": ["docs"], + "owns": "User and contributor documentation and agent guidance files"}, + {"name": "Tooling", "prefix": "()", "types": ["build", "ci", "chore", "docs"], + "owns": "Everything else: build system, CI configuration, scripts, container builds, helper modules, AI agent tooling"}, +] + +# Group of each binary built by `make build`. Generation fails for a binary missing here. +BINARY_GROUPS = { + "hypershift": "CLI", + "product-cli": "CLI", + "hypershift-operator": "HO", + "karpenter-operator": "HO", + "control-plane-operator": "CPO", + "control-plane-pki-operator": "CPO", +} +# The hypershift CLI is built from the repository root; its commands live in these directories. +ROOT_BINARY_SOURCES = ("cmd",) +# Group for top-level Go code that binaries of more than one group import. +SHARED_GROUP = "HO" +# Module definitions by module directory ("" is the repository root). Other modules follow +# the group of the directory that contains them. +MODULE_GROUPS = {"api": "API", "": "Dependencies", "hack/tools": "Dependencies"} +WORKSPACE_GROUP = "Dependencies" +DOC_DIRS = ("docs", "examples") +# Conventions that win over generated-file and directory rules, in order. +CONVENTIONS = [ + ("**/AGENTS.md", "Docs", "agent guidance"), + ("**/CLAUDE.md", "Docs", "agent guidance"), + ("test/**", "E2E", "e2e, integration, and envtest suites"), + ("api/**/*_test.go", "E2E", "API validation tests"), +] +# Path conventions for generated output, applied after per-file exceptions. +GENERATED_PATTERNS = ["**/zz_generated*", "**/zz_generated*/**"] + +# ---- FACTS ------------------------------------------------------------------------------- + +GENERATED_GO = re.compile(rb"^// Code generated .* DO NOT EDIT\.$", re.M) +CRD_MARKER = b"controller-gen.kubebuilder.io/version" +MAKE_OUTPUT_DIRS = [ + re.compile(r"output:crd:artifacts:config=(\S+)"), + re.compile(r"^\tcp \S+ (\S+?)/?$", re.M), + re.compile(r"^\tmv \S+ (\S+?)/?$", re.M), +] + + +@dataclass +class Facts: + paths: list[str] + binaries: dict[str, tuple[str, str]] # make target -> (group, package directory, "" for the root) + importers: dict[str, set[str]] = field(default_factory=dict) # top-level directory -> importing groups + generated: set[str] = field(default_factory=set) # paths whose content is marked as generated + output_dirs: set[str] = field(default_factory=set) # directories Makefile recipes write generated files to + + +def git(*args: str) -> str: + return subprocess.run(["git", *args], check=True, stdout=subprocess.PIPE, text=True).stdout + + +def read_head(path: str, size: int = 4096) -> bytes: + try: + with open(path, "rb") as f: + return f.read(size) + except OSError: + return b"" + + +def parse_binaries(makefile: str) -> dict[str, tuple[str, str]]: + build = re.search(r"^build:(.*)$", makefile, re.M) + if not build: + raise SystemExit("error: Makefile has no build target") + binaries = {} + for target in build.group(1).split(): + recipe = re.search(rf"^{re.escape(target)}:.*\n((?:\t.*\n)+)", makefile, re.M) + out = recipe and re.search(r"-o \S+ (\.\S*)\s*$", recipe.group(1), re.M) + if not out: + raise SystemExit(f"error: cannot find the package built by make {target}") + if target not in BINARY_GROUPS: + raise SystemExit(f"error: add make {target} to BINARY_GROUPS in {Path(__file__).name}") + binaries[target] = (BINARY_GROUPS[target], out.group(1).removeprefix(".").strip("/")) + return binaries + + +def parse_output_dirs(makefile: str) -> set[str]: + dirs = set() + for pattern in MAKE_OUTPUT_DIRS: + for d in pattern.findall(makefile): + d = d.removeprefix("./").rstrip("/") + if "$" not in d and "*" not in d and not d.startswith(("/", ".")) and Path(d).is_dir(): + dirs.add(d) + return dirs + + +def is_generated(path: str) -> bool: + if path.endswith(".go"): + return bool(GENERATED_GO.search(read_head(path, 2048))) + if path.endswith((".yaml", ".yml")): + return CRD_MARKER in read_head(path) + return False + + +def collect_facts() -> Facts: + raw = git("ls-files", "-z", "--cached", "--others", "--exclude-standard") + paths = sorted({p for p in raw.split("\0") if p and Path(p).exists()}) + makefile = Path("Makefile").read_text() + facts = Facts(paths=paths, binaries=parse_binaries(makefile), output_dirs=parse_output_dirs(makefile)) + importers: dict[str, set[str]] = defaultdict(set) + for group, pkg_dir in facts.binaries.values(): + for pkg in go_deps(pkg_dir): + if pkg.startswith(GO_MODULE): + importers[pkg[len(GO_MODULE):].split("/")[0]].add(group) + facts.importers = dict(importers) + facts.generated = {p for p in paths if "/vendor/" not in f"/{p}" and is_generated(p)} + return facts + + +def go_deps(pkg_dir: str) -> list[str]: + env = {**os.environ, "GO111MODULE": "on", "GOWORK": "off", "GOFLAGS": "-mod=vendor"} # as the Makefile's GO + out = subprocess.run(["go", "list", "-deps", f"./{pkg_dir}"], check=True, stdout=subprocess.PIPE, text=True, + env=env) + return out.stdout.split() + + +# ---- RULES ------------------------------------------------------------------------------- + + +def rule(pattern: str, group: str, why: str) -> dict[str, str]: + return {"pattern": pattern, "group": group, "why": why} + + +def top_level_group(top: str, is_dir: bool, facts: Facts) -> tuple[str, str]: + """Return the structural group of a top-level entry and the reason.""" + if not is_dir: + if top.endswith(".md"): + return "Docs", "root documentation" + if top.endswith(".go") and any(d == "" for _, d in facts.binaries.values()): + group = next(g for g, d in facts.binaries.values() if d == "") + return group, "source of the binary built from the repository root" + return "Tooling", "root file" + if top in DOC_DIRS: + return "Docs", "documentation" + if top in MODULE_GROUPS: + return MODULE_GROUPS[top], f"module {top}/" + for target, (group, pkg_dir) in facts.binaries.items(): + if pkg_dir == top: + return group, f"package of make {target}" + if pkg_dir == "" and top in ROOT_BINARY_SOURCES: + return group, f"commands of make {target}" + groups = facts.importers.get(top, set()) + if len(groups) == 1: + group = next(iter(groups)) + return group, f"imported only by {group} binaries" + if groups: + return SHARED_GROUP, f"shared by {', '.join(sorted(groups))} binaries" + return "Tooling", "not part of any binary" + + +def collapse(paths: set[str], universe: list[str]) -> list[str]: + """Express paths as the fewest dir/** patterns and file paths that match nothing else in universe.""" + children: dict[str, set[str]] = defaultdict(set) + for p in universe: + parts = p.split("/") + for i in range(1, len(parts)): + children["/".join(parts[:i])].add(p) + full = {d for d, members in children.items() if members <= paths} + patterns = [f"{d}/**" for d in sorted(full) if not any(d.startswith(f"{o}/") for o in full)] + covered = {p for d in patterns for p in children[d[:-3]]} + return patterns + sorted(paths - covered) + + +def build_rules(facts: Facts) -> list[dict[str, str]]: + paths = facts.paths + rules: list[dict[str, str]] = [] + + def first_match(path: str) -> dict[str, str] | None: + return next((r for r in rules if match_path(r["pattern"], path)), None) + + vendored = sorted(p.removesuffix("vendor/modules.txt") for p in paths if p.endswith("vendor/modules.txt") + and p.count("vendor/") == 1) + modules = sorted(p.removesuffix("go.mod").rstrip("/") for p in paths if p.endswith("go.mod") + and "vendor/" not in p) + testsuite_dirs = sorted({p.rsplit("/tests/", 1)[0] + "/tests" for p in paths + if p.endswith(".testsuite.yaml") and "/tests/" in p}) + tops = {p.split("/")[0]: "/" in p for p in paths} + + for v in vendored: + rules.append(rule(f"{v}vendor/**", "Vendor", f"vendor tree of module {v.rstrip('/') or 'root'}")) + # Hand-written files inside generated-output paths keep their directory's group. + for p in paths: + if (p.endswith(".go") and p not in facts.generated and any(match_path(g, p) for g in GENERATED_PATTERNS) + and not first_match(p)): + group, why = top_level_group(p.split("/")[0], True, facts) + rules.append(rule(p, group, f"hand-written file in generated output; {why}")) + rules.extend(rule(p, g, why) for p, g, why in CONVENTIONS) + rules.extend(rule(f"{d}/**", "E2E", "envtest suites") for d in testsuite_dirs) + rules.extend(rule(p, "Vendor", "zz_generated output") for p in GENERATED_PATTERNS) + + out_dirs = sorted(d for d in facts.output_dirs if not first_match(f"{d}/x") + and not any(d.startswith(f"{o}/") for o in facts.output_dirs)) + for d in out_dirs: + for p in paths: + if p.startswith(f"{d}/") and p.endswith(".go") and p not in facts.generated and not first_match(p): + group, why = top_level_group(p.split("/")[0], True, facts) + rules.append(rule(p, group, f"hand-written file in generated output; {why}")) + rules.extend(rule(f"{d}/**", "Vendor", "generated by a Makefile recipe") for d in out_dirs) + + marked = {p for p in facts.generated if not first_match(p)} + rules.extend(rule(p, "Vendor", "marked as generated") for p in collapse(marked, paths)) + + for m in modules: + group = MODULE_GROUPS.get(m) + if group: + for name in ("go.mod", "go.sum"): + rules.append(rule(f"{m}/{name}" if m else name, group, f"definition of module {m or 'root'}")) + for p in paths: + if p.rsplit("/", 1)[-1] in ("go.work", "go.work.sum") and not first_match(p): + rules.append(rule(p, WORKSPACE_GROUP, "Go workspace definition")) + + for top, is_dir in sorted(tops.items()): + pattern = f"{top}/**" if is_dir else top + if any(r["pattern"] == pattern for r in rules): + continue + group, why = top_level_group(top, is_dir, facts) + rules.append(rule(pattern, group, why)) + return rules + + +def ownership_document(facts: Facts) -> dict: + return { + "description": "GENERATED by skills/restructure-commits/generate_ownership.py (make update); do not edit. " + "Groups are listed in commit order. Rules are evaluated top to bottom and the first matching " + "pattern wins. Patterns are anchored at the repository root; '**' matches zero or more path " + "segments and other wildcards never cross '/'.", + "groups": GROUPS, + "rules": build_rules(facts), + } + + +def to_json(document: dict) -> str: + """Serialize with one group or rule per line so regenerated diffs stay reviewable.""" + def items(key: str) -> str: + return ",\n".join(f" {json.dumps(item)}" for item in document[key]) + return (f'{{\n "description": {json.dumps(document["description"])},\n' + f' "groups": [\n{items("groups")}\n ],\n "rules": [\n{items("rules")}\n ]\n}}\n') + + +def render_table() -> str: + lines = [ + BEGIN_MARKER, + "| Order | Group | Commit prefix | Allowed `` | Owns |", + "|-------|-------|---------------|------------------|------|", + ] + for i, g in enumerate(GROUPS, 1): + types = ", ".join(f"`{t}`" for t in g["types"]) if "" in g["prefix"] else "fixed" + lines.append(f"| {i} | {g['name']} | `{g['prefix']}` | {types} | {g['owns']} |") + lines.append(END_MARKER) + return "\n".join(lines) + + +def rendered_skill(text: str) -> str: + """Return SKILL.md text with the generated table replaced.""" + start = text.find(BEGIN_MARKER) + end = text.find(END_MARKER, start + len(BEGIN_MARKER)) if start >= 0 else -1 + if start < 0 or end < 0: + raise SystemExit(f"error: generated table markers not found in {SKILL_FILE}") + return text[:start] + render_table() + text[end + len(END_MARKER):] + + +def main() -> int: + if not Path("Makefile").is_file() or not Path("go.mod").is_file(): + print("error: run from the repository root", file=sys.stderr) + return 2 + RULES_FILE.write_text(to_json(ownership_document(collect_facts()))) + SKILL_FILE.write_text(rendered_skill(SKILL_FILE.read_text())) + return 0 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/skills/restructure-commits/generate_ownership_test.py b/skills/restructure-commits/generate_ownership_test.py new file mode 100644 index 000000000000..793347c39fd3 --- /dev/null +++ b/skills/restructure-commits/generate_ownership_test.py @@ -0,0 +1,100 @@ +"""Tests for generate_ownership.py; run with `make test-commit-layout`.""" + +from __future__ import annotations + +import sys +from pathlib import Path + +import pytest + +sys.path.insert(0, str(Path(__file__).resolve().parent)) + +import commit_layout # noqa: E402 +import generate_ownership # noqa: E402 +from generate_ownership import Facts # noqa: E402 + +BINARIES = { + "hypershift": ("CLI", ""), + "hypershift-operator": ("HO", "hypershift-operator"), + "control-plane-operator": ("CPO", "control-plane-operator"), +} +PATHS = [ + "main.go", "go.mod", "go.sum", "Makefile", "README.md", + "api/go.mod", "api/go.sum", "api/v1/types.go", "api/v1/types_test.go", "api/v1/zz_generated.deepcopy.go", + "api/vendor/modules.txt", "api/vendor/k8s.io/api/zz_generated.deepcopy.go", + "vendor/modules.txt", "vendor/k8s.io/api/x.go", + "client/clientset.go", "client/informers.go", + "cmd/install/install.go", "cmd/infra/client.go", "cmd/infra/create.go", + "cmd/crds/doc.go", "cmd/crds/hostedclusters.yaml", "cmd/crds/tests/hc/stable.testsuite.yaml", + "control-plane-operator/main.go", "hypershift-operator/main.go", + "sidecar/main.go", "support/util.go", "brand-new-dir/main.go", + "docs/index.md", "pkg/AGENTS.md", "test/e2e/e2e_test.go", +] + + +def classify(facts: Facts) -> dict[str, str]: + groups = [commit_layout.Group(i + 1, g["name"], g["prefix"]) for i, g in enumerate(generate_ownership.GROUPS)] + rules = [commit_layout.Rule(r["pattern"], r["group"]) for r in generate_ownership.build_rules(facts)] + ownership = commit_layout.Ownership(groups, rules) + return {p: ownership.classify(p).name for p in facts.paths} + + +@pytest.fixture +def facts() -> Facts: + return Facts( + paths=PATHS, + binaries=BINARIES, + importers={"sidecar": {"CPO"}, "support": {"CPO", "HO"}, "cmd": {"CLI", "HO"}}, + generated={"api/v1/zz_generated.deepcopy.go", "client/clientset.go", "client/informers.go", + "cmd/infra/client.go", "cmd/crds/hostedclusters.yaml"}, + output_dirs={"cmd/crds"}, + ) + + +def test_components_follow_binaries_and_their_imports(facts: Facts) -> None: + got = classify(facts) + assert got["main.go"] == "CLI" + assert got["cmd/install/install.go"] == "CLI" + assert got["hypershift-operator/main.go"] == "HO" + assert got["control-plane-operator/main.go"] == "CPO" + assert got["sidecar/main.go"] == "CPO", "imported only by CPO binaries" + assert got["support/util.go"] == "HO", "shared by several groups" + assert got["brand-new-dir/main.go"] == "Tooling", "not part of any binary" + + +def test_generated_output_is_vendor_but_hand_written_files_keep_their_group(facts: Facts) -> None: + got = classify(facts) + assert got["client/clientset.go"] == "Vendor" + assert got["cmd/infra/client.go"] == "Vendor" + assert got["cmd/infra/create.go"] == "CLI" + assert got["api/v1/zz_generated.deepcopy.go"] == "Vendor" + assert got["cmd/crds/hostedclusters.yaml"] == "Vendor" + assert got["cmd/crds/doc.go"] == "CLI" + assert got["cmd/crds/tests/hc/stable.testsuite.yaml"] == "E2E" + assert got["api/vendor/k8s.io/api/zz_generated.deepcopy.go"] == "Vendor" + + +def test_modules_conventions_and_documentation(facts: Facts) -> None: + got = classify(facts) + assert (got["go.mod"], got["go.sum"], got["api/go.mod"], got["api/go.sum"]) == ( + "Dependencies", "Dependencies", "API", "API") + assert (got["vendor/modules.txt"], got["api/vendor/modules.txt"]) == ("Vendor", "Vendor") + assert got["api/v1/types.go"] == "API" + assert got["api/v1/types_test.go"] == "E2E" + assert got["test/e2e/e2e_test.go"] == "E2E" + assert (got["README.md"], got["docs/index.md"], got["pkg/AGENTS.md"]) == ("Docs", "Docs", "Docs") + assert got["Makefile"] == "Tooling" + + +def test_skill_table_replaces_only_the_generated_block() -> None: + begin, end = generate_ownership.BEGIN_MARKER, generate_ownership.END_MARKER + text = f"intro {end}\n{begin}\nstale\n{end}\noutro\n" + assert generate_ownership.rendered_skill(text) == f"intro {end}\n{generate_ownership.render_table()}\noutro\n" + + +def test_new_binaries_need_a_group(monkeypatch: pytest.MonkeyPatch) -> None: + makefile = "build: new-operator\n\nnew-operator:\n\t$(GO) build -o bin/new-operator ./new-operator\n" + with pytest.raises(SystemExit, match="BINARY_GROUPS"): + generate_ownership.parse_binaries(makefile) + monkeypatch.setitem(generate_ownership.BINARY_GROUPS, "new-operator", "HO") + assert generate_ownership.parse_binaries(makefile) == {"new-operator": ("HO", "new-operator")} diff --git a/skills/restructure-commits/ownership.json b/skills/restructure-commits/ownership.json new file mode 100644 index 000000000000..297fde919fc8 --- /dev/null +++ b/skills/restructure-commits/ownership.json @@ -0,0 +1,138 @@ +{ + "description": "GENERATED by skills/restructure-commits/generate_ownership.py (make update); do not edit. Groups are listed in commit order. Rules are evaluated top to bottom and the first matching pattern wins. Patterns are anchored at the repository root; '**' matches zero or more path segments and other wildcards never cross '/'.", + "groups": [ + {"name": "API", "prefix": "(api)", "types": ["feat", "fix", "build", "refactor"], "owns": "API types, markers, and the api/ module definition (api/go.mod and api/go.sum stay together)"}, + {"name": "Dependencies", "prefix": "build(deps)", "types": ["build"], "owns": "Root, tools, and workspace module definitions; never mixed with generated vendor trees"}, + {"name": "Vendor", "prefix": "chore(api)", "types": ["chore"], "owns": "Generated output: vendor trees, clients, zz_generated files, generated CRD manifests, and other files marked as generated"}, + {"name": "CLI", "prefix": "(cli)", "types": ["feat", "fix", "refactor", "test"], "owns": "hypershift and hcp CLIs, with their tests, testdata, and fixtures"}, + {"name": "HO", "prefix": "(hypershift-operator)", "types": ["feat", "fix", "refactor", "perf", "test"], "owns": "hypershift-operator binaries and Go code shared by binaries of more than one group"}, + {"name": "CPO", "prefix": "(control-plane-operator)", "types": ["feat", "fix", "refactor", "perf", "test"], "owns": "control-plane-operator binaries and the Go code only they use, such as control plane sidecars"}, + {"name": "E2E", "prefix": "test(e2e)", "types": ["test"], "owns": "E2E, integration, and envtest suites, including API validation tests"}, + {"name": "Docs", "prefix": "docs", "types": ["docs"], "owns": "User and contributor documentation and agent guidance files"}, + {"name": "Tooling", "prefix": "()", "types": ["build", "ci", "chore", "docs"], "owns": "Everything else: build system, CI configuration, scripts, container builds, helper modules, AI agent tooling"} + ], + "rules": [ + {"pattern": "vendor/**", "group": "Vendor", "why": "vendor tree of module root"}, + {"pattern": "api/vendor/**", "group": "Vendor", "why": "vendor tree of module api"}, + {"pattern": "hack/tools/vendor/**", "group": "Vendor", "why": "vendor tree of module hack/tools"}, + {"pattern": "hack/tools/deadcode-report/testdata/reachability/vendor/**", "group": "Vendor", "why": "vendor tree of module hack/tools/deadcode-report/testdata/reachability"}, + {"pattern": "cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/doc.go", "group": "CLI", "why": "hand-written file in generated output; commands of make hypershift"}, + {"pattern": "**/AGENTS.md", "group": "Docs", "why": "agent guidance"}, + {"pattern": "**/CLAUDE.md", "group": "Docs", "why": "agent guidance"}, + {"pattern": "test/**", "group": "E2E", "why": "e2e, integration, and envtest suites"}, + {"pattern": "api/**/*_test.go", "group": "E2E", "why": "API validation tests"}, + {"pattern": "cmd/install/assets/crds/hypershift-operator/tests/**", "group": "E2E", "why": "envtest suites"}, + {"pattern": "karpenter-operator/controllers/karpenter/assets/tests/**", "group": "E2E", "why": "envtest suites"}, + {"pattern": "**/zz_generated*", "group": "Vendor", "why": "zz_generated output"}, + {"pattern": "**/zz_generated*/**", "group": "Vendor", "why": "zz_generated output"}, + {"pattern": "karpenter-operator/controllers/karpenter/assets/assets.go", "group": "HO", "why": "hand-written file in generated output; package of make karpenter-operator"}, + {"pattern": "cmd/install/assets/crds/hypershift-operator/**", "group": "Vendor", "why": "generated by a Makefile recipe"}, + {"pattern": "karpenter-operator/controllers/karpenter/assets/**", "group": "Vendor", "why": "generated by a Makefile recipe"}, + {"pattern": "client/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api-provider-agent/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api-provider-aws/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api-provider-azure/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api-provider-gcp/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api-provider-ibmcloud/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api-provider-kubevirt/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/cluster-api-provider-openstack/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/install/assets/crds/external-dns/**", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "cmd/infra/aws/delegating_client.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "hack/tools/deadcode-report/testdata/reachability/generated.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "support/awsapi/ec2.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "support/awsapi/elasticloadbalancing.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "support/awsapi/elasticloadbalancingv2.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "support/awsapi/iam.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "support/awsapi/route53.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "support/awsapi/s3.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "support/awsapi/sqs.go", "group": "Vendor", "why": "marked as generated"}, + {"pattern": "go.mod", "group": "Dependencies", "why": "definition of module root"}, + {"pattern": "go.sum", "group": "Dependencies", "why": "definition of module root"}, + {"pattern": "api/go.mod", "group": "API", "why": "definition of module api"}, + {"pattern": "api/go.sum", "group": "API", "why": "definition of module api"}, + {"pattern": "hack/tools/go.mod", "group": "Dependencies", "why": "definition of module hack/tools"}, + {"pattern": "hack/tools/go.sum", "group": "Dependencies", "why": "definition of module hack/tools"}, + {"pattern": "hack/workspace/go.work", "group": "Dependencies", "why": "Go workspace definition"}, + {"pattern": ".actionlint.yaml", "group": "Tooling", "why": "root file"}, + {"pattern": ".chai-bot/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": ".ci-operator.yaml", "group": "Tooling", "why": "root file"}, + {"pattern": ".claude/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": ".claude-plugin/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": ".coderabbit.yaml", "group": "Tooling", "why": "root file"}, + {"pattern": ".codespellignore", "group": "Tooling", "why": "root file"}, + {"pattern": ".cursor/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": ".dockerignore", "group": "Tooling", "why": "root file"}, + {"pattern": ".gitattributes", "group": "Tooling", "why": "root file"}, + {"pattern": ".github/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": ".gitignore", "group": "Tooling", "why": "root file"}, + {"pattern": ".gitleaks.toml", "group": "Tooling", "why": "root file"}, + {"pattern": ".gitlint", "group": "Tooling", "why": "root file"}, + {"pattern": ".golangci.yml", "group": "Tooling", "why": "root file"}, + {"pattern": ".pi", "group": "Tooling", "why": "root file"}, + {"pattern": ".pre-commit-config.yaml", "group": "Tooling", "why": "root file"}, + {"pattern": ".snyk", "group": "Tooling", "why": "root file"}, + {"pattern": ".tekton/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": ".testcoverage.yml", "group": "Tooling", "why": "root file"}, + {"pattern": "AGENTS.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "ARCHITECTURE.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "CLAUDE.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "CONTRIBUTING.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "Containerfile.cli", "group": "Tooling", "why": "root file"}, + {"pattern": "Containerfile.control-plane", "group": "Tooling", "why": "root file"}, + {"pattern": "Containerfile.operator", "group": "Tooling", "why": "root file"}, + {"pattern": "DEVELOPMENT.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "Dockerfile", "group": "Tooling", "why": "root file"}, + {"pattern": "Dockerfile.control-plane", "group": "Tooling", "why": "root file"}, + {"pattern": "Dockerfile.dev", "group": "Tooling", "why": "root file"}, + {"pattern": "Dockerfile.e2e", "group": "Tooling", "why": "root file"}, + {"pattern": "Dockerfile.github-actions-runner", "group": "Tooling", "why": "root file"}, + {"pattern": "GEMINI.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "HACKING.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "LICENSE", "group": "Tooling", "why": "root file"}, + {"pattern": "Makefile", "group": "Tooling", "why": "root file"}, + {"pattern": "OWNERS", "group": "Tooling", "why": "root file"}, + {"pattern": "OWNERS_ALIASES", "group": "Tooling", "why": "root file"}, + {"pattern": "README.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "TESTING.md", "group": "Docs", "why": "root documentation"}, + {"pattern": "api/**", "group": "API", "why": "module api/"}, + {"pattern": "availability-prober/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "cliff.toml", "group": "Tooling", "why": "root file"}, + {"pattern": "cmd/**", "group": "CLI", "why": "commands of make hypershift"}, + {"pattern": "codecov.yml", "group": "Tooling", "why": "root file"}, + {"pattern": "contrib/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": "control-plane-operator/**", "group": "CPO", "why": "package of make control-plane-operator"}, + {"pattern": "control-plane-pki-operator/**", "group": "CPO", "why": "package of make control-plane-pki-operator"}, + {"pattern": "dnsresolver/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "docs/**", "group": "Docs", "why": "documentation"}, + {"pattern": "etcd-backup/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "etcd-defrag/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "etcd-recovery/**", "group": "HO", "why": "imported only by HO binaries"}, + {"pattern": "etcd-upload/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "examples/**", "group": "Docs", "why": "documentation"}, + {"pattern": "hack/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": "hypershift-ci-python/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": "hypershift-operator/**", "group": "HO", "why": "package of make hypershift-operator"}, + {"pattern": "ignition-server/**", "group": "HO", "why": "shared by CPO, HO binaries"}, + {"pattern": "karpenter-operator/**", "group": "HO", "why": "package of make karpenter-operator"}, + {"pattern": "kas-bootstrap/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "konnectivity-https-proxy/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "konnectivity-socks5-proxy/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "kubernetes-default-proxy/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "kubevirtexternalinfra/**", "group": "HO", "why": "shared by CLI, CPO, HO binaries"}, + {"pattern": "main.go", "group": "CLI", "why": "source of the binary built from the repository root"}, + {"pattern": "pkg/**", "group": "HO", "why": "shared by CLI, CPO, HO binaries"}, + {"pattern": "product-cli/**", "group": "CLI", "why": "package of make product-cli"}, + {"pattern": "release-manifests/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": "releases/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": "renovate.json", "group": "Tooling", "why": "root file"}, + {"pattern": "shared-ingress/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": "sharedingress-config-generator/**", "group": "HO", "why": "imported only by HO binaries"}, + {"pattern": "skills/**", "group": "Tooling", "why": "not part of any binary"}, + {"pattern": "staticcheck.conf", "group": "Tooling", "why": "root file"}, + {"pattern": "support/**", "group": "HO", "why": "shared by CLI, CPO, HO binaries"}, + {"pattern": "sync-fg-configmap/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "sync-global-pullsecret/**", "group": "CPO", "why": "imported only by CPO binaries"}, + {"pattern": "token-minter/**", "group": "CPO", "why": "imported only by CPO binaries"} + ] +} diff --git a/skills/restructure-commits/sample_branch.py b/skills/restructure-commits/sample_branch.py new file mode 100644 index 000000000000..8338d1a03b03 --- /dev/null +++ b/skills/restructure-commits/sample_branch.py @@ -0,0 +1,173 @@ +#!/usr/bin/env python3 +"""Create a reproducible sample repository that reproduces the PR #9568 layout mistake. + +The feature branch mixes module definitions with generated vendor trees, the same way +openshift/hypershift#9568 did, and adds a rename with spaces, a deletion, a component +fixture, envtest suites, CI/Makefile edits, and agent docs. The repository also contains +a copy of this skill so agents can follow SKILL.md with repository-relative paths. + +Usage: python3 sample_branch.py +Result: /origin.git (bare remote) and /work (clone on branch k8s-1.37-bump). +""" + +from __future__ import annotations + +import os +import shutil +import subprocess +import sys +from pathlib import Path + +SKILL_DIR = Path(__file__).resolve().parent +REPO_ROOT = SKILL_DIR.parents[1] +FEATURE_BRANCH = "k8s-1.37-bump" + +BASE_FILES = { + "go.mod": "module github.com/openshift/hypershift\n\nrequire k8s.io/api v0.36.0\n", + "go.sum": "k8s.io/api v0.36.0 h1:base\n", + "vendor/modules.txt": "# k8s.io/api v0.36.0\n", + "vendor/k8s.io/api/core/v1/types.go": "package v1 // v0.36.0\n", + "api/go.mod": "module github.com/openshift/hypershift/api\n\nrequire k8s.io/api v0.36.0\n", + "api/go.sum": "k8s.io/api v0.36.0 h1:base\n", + "api/vendor/modules.txt": "# k8s.io/api v0.36.0\n", + "api/vendor/k8s.io/api/core/v1/types.go": "package v1 // v0.36.0\n", + "api/hypershift/v1beta1/hostedcluster_types.go": "package v1beta1\n", + "api/hypershift/v1beta1/zz_generated.deepcopy.go": "package v1beta1 // generated\n", + "cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters.crd.yaml": "kind: CRD # 1.36\n", + "control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/testdata/zz_fixture_TestKubeScheduler.yaml": + "version: 1.36\n", + "hypershift-operator/controllers/legacy/legacy.go": "package legacy\n", + "docs/content/how-to/k8s old guide.md": "# Kubernetes guide\n", + "test/envtest/README.md": "# envtest\n", + ".github/workflows/envtest-kube-reusable.yaml": "matrix: [1.36]\n", + "Makefile": "KUBE_VERSIONS := 1.36\n", + "AGENTS.md": "# Agents\n", +} + +FEATURE_COMMITS = [ + ( + "build(api): bump Kubernetes dependencies to 1.37", + { + "api/go.mod": "module github.com/openshift/hypershift/api\n\nrequire k8s.io/api v0.37.0\n", + "api/go.sum": "k8s.io/api v0.37.0 h1:new\n", + "api/vendor/modules.txt": "# k8s.io/api v0.37.0\n", + "api/vendor/k8s.io/api/core/v1/types.go": "package v1 // v0.37.0\n", + }, + [], + ), + ( + "chore(api): regenerate Kubernetes 1.37 dependencies and CRDs", + { + "go.mod": "module github.com/openshift/hypershift\n\nrequire k8s.io/api v0.37.0\n", + "go.sum": "k8s.io/api v0.37.0 h1:new\n", + "vendor/modules.txt": "# k8s.io/api v0.37.0\n", + "vendor/k8s.io/api/core/v1/types.go": "package v1 // v0.37.0\n", + "api/hypershift/v1beta1/zz_generated.deepcopy.go": "package v1beta1 // generated 1.37\n", + "cmd/install/assets/crds/hypershift-operator/zz_generated.crd-manifests/hostedclusters.crd.yaml": + "kind: CRD # 1.37\n", + }, + [], + ), + ( + "test(control-plane-operator): refresh Kubernetes 1.37 scheduler fixtures", + { + "control-plane-operator/controllers/hostedcontrolplane/v2/kube_scheduler/testdata/zz_fixture_TestKubeScheduler.yaml": + "version: 1.37\n", + "docs/content/how-to/k8s new guide.md": "# Kubernetes guide\n", + }, + ["hypershift-operator/controllers/legacy/legacy.go", "docs/content/how-to/k8s old guide.md"], + ), + ( + "test(e2e): add Kubernetes 1.37 envtest coverage", + { + "cmd/install/assets/crds/hypershift-operator/tests/hostedclusters.hypershift.openshift.io/k8s-1.37.testsuite.yaml": + "name: k8s 1.37\n", + "test/envtest/README.md": "# envtest\n\nCovers 1.30-1.37.\n", + ".github/workflows/envtest-kube-reusable.yaml": "matrix: [1.36, 1.37]\n", + "Makefile": "KUBE_VERSIONS := 1.36 1.37\n", + "AGENTS.md": "# Agents\n\nEnvtest covers Kubernetes 1.37.\n", + }, + [], + ), +] + +SUPPORT_FILES = [ + "skills/restructure-commits/SKILL.md", + "skills/restructure-commits/ownership.json", + "skills/restructure-commits/commit_layout.py", + "skills/git-commit-format/SKILL.md", + ".gitlint", +] + + +def clean_env() -> dict[str, str]: + """Return the environment without GIT_* overrides, such as the GIT_DIR and GIT_INDEX_FILE set by Git hooks.""" + return {k: v for k, v in os.environ.items() if not k.startswith("GIT_")} + + +def run(cwd: Path, *args: str) -> str: + return subprocess.run(args, cwd=cwd, check=True, stdout=subprocess.PIPE, text=True, env=clean_env()).stdout + + +def write(root: Path, files: dict[str, str]) -> None: + for rel, content in files.items(): + path = root / rel + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(content) + + +def init_repo(repo: Path) -> None: + """Create a repository on main with a fixed identity, no signing, and no hooks.""" + repo.mkdir(parents=True, exist_ok=True) + run(repo, "git", "init", "-q", "-b", "main") + for key, value in ( + ("user.name", "Sample Author"), + ("user.email", "sample@example.com"), + ("commit.gpgsign", "false"), + ("core.hooksPath", os.devnull), + ): + run(repo, "git", "config", key, value) + + +def commit(repo: Path, message: str, files: dict[str, str] | None = None, delete: tuple[str, ...] = ()) -> str: + """Write files, delete paths, commit everything, and return the new commit SHA.""" + write(repo, files or {}) + for rel in delete: + (repo / rel).unlink() + run(repo, "git", "add", "-A") + run(repo, "git", "commit", "-q", "-s", "-m", message) + return run(repo, "git", "rev-parse", "HEAD").strip() + + +def create_sample(target: Path) -> Path: + target = Path(target).resolve() + origin, work = target / "origin.git", target / "work" + origin.mkdir(parents=True, exist_ok=True) + run(origin, "git", "init", "-q", "--bare", "-b", "main") + init_repo(work) + run(work, "git", "remote", "add", "origin", str(origin)) + + write(work, BASE_FILES) + for rel in SUPPORT_FILES: + dst = work / rel + dst.parent.mkdir(parents=True, exist_ok=True) + shutil.copyfile(REPO_ROOT / rel, dst) + commit(work, "chore: sample base") + run(work, "git", "push", "-q", "origin", "main") + + run(work, "git", "checkout", "-q", "-b", FEATURE_BRANCH) + for message, files, deletions in FEATURE_COMMITS: + commit(work, message, files, tuple(deletions)) + run(work, "git", "push", "-q", "-u", "origin", FEATURE_BRANCH) + return work + + +if __name__ == "__main__": + if len(sys.argv) != 2: + print(__doc__, file=sys.stderr) + sys.exit(2) + dest = Path(sys.argv[1]) + if dest.exists() and any(dest.iterdir()): + print(f"error: {dest} is not empty", file=sys.stderr) + sys.exit(2) + print(create_sample(dest))