Skip to content

CNTRLPLANE-3642: ci: add top-level directory drift detection #8773

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mgencur wants to merge 1 commit into from
Closed

CNTRLPLANE-3642: ci: add top-level directory drift detection #8773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ verify-codecov: ## Validate codecov.yml against Codecov's API.
| tee /dev/stderr | grep -q "^Valid!"

.PHONY: verify-parallel
verify-parallel: verify-codespell verify-codecov verify-api-deps lint cpo-container-sync run-gitlint verify-docs-nav
verify-parallel: verify-codespell verify-codecov verify-api-deps lint cpo-container-sync run-gitlint verify-docs-nav verify-toplevel-dirs

.PHONY: verify
verify: generate update staticcheck fmt vet
Expand Down Expand Up @@ -607,6 +607,10 @@ verify-codespell: codespell ## Verify codespell.
verify-api-deps: $(VERIFY_API_DEPS) ## Verify API dependencies against allowlist.
@$(VERIFY_API_DEPS)

.PHONY: verify-toplevel-dirs
verify-toplevel-dirs: ## Verify top-level directory list is in sync with hack/ci/toplevel-dirs.txt.
@bash hack/ci/verify-toplevel-dirs.sh

.PHONY: run-gitlint
run-gitlint: $(GITLINT)
ifdef PULL_BASE_SHA
Expand Down
34 changes: 34 additions & 0 deletions hack/ci/toplevel-dirs.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
api

@mehabhalodiya mehabhalodiya Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The file is a plain list with no context. Someone encountering it cold won't know what it's for, why it exists, or what to do when they add a directory. A two-line comment at the top would save confusion:

# Tracked list of top-level directories in this repo.
# Used by hack/ci/verify-toplevel-dirs.sh to detect drift in the CI skip pattern.
# When adding a new top-level directory, update this file AND the pipeline_skip_if_only_changed
# regex in ci-operator (unless the new directory contains E2E tests — see CNTRLPLANE-3642).

@mgencur mgencur Jun 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is read by a script so I can't easily add the description. Also, next to this file is verify-toplevel-dirs.sh which has this text:

# Verifies that the tracked top-level directory list stays in sync with
# the actual git-tracked directories. This prevents the CI skip pattern
# (pipeline_skip_if_only_changed) from silently going stale when new
# top-level directories are added to the repo.

Isn't it enough? I could possibly add a README.txt to this folder and mention this file but since it's already mentioned in the script that is named very similarly, I thought this would be enough. Please let me know.

@mehabhalodiya mehabhalodiya Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I missed it. I guess it would be fine then :)

availability-prober
client
cmd
contrib
control-plane-operator
control-plane-pki-operator
dnsresolver
docs
etcd-backup
etcd-defrag
etcd-recovery
etcd-upload
examples
hack
hypershift-ci-python
hypershift-operator
ignition-server
karpenter-operator
kas-bootstrap
konnectivity-https-proxy
konnectivity-socks5-proxy
kubernetes-default-proxy
kubevirtexternalinfra
pkg
product-cli
shared-ingress
sharedingress-config-generator
support
sync-fg-configmap
sync-global-pullsecret
test

@mehabhalodiya mehabhalodiya Jun 19, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test/ is in toplevel-dirs.txt — but NOT in the CI skip pattern, and the script doesn't enforce that distinction:
hack/ci/toplevel-dirs.txt line 32 includes test. The verify script checks that the tracked list matches actual top-level directories — that's correct. But the pipeline_skip_if_only_changed regex in PR #80732 intentionally excludes test/ because it contains E2E tests.

This means:

  • Someone adds a new top-level directory → verify fails → they update toplevel-dirs.txt ✅
  • They are then also told (by the error message on line 29) to update the CI operator regex ✅
  • But the script has no way to distinguish "this dir should be in the regex" from "this dir is intentionally excluded from the regex (like test/)"

The error message on line 29 says:

"Also update the pipeline_skip_if_only_changed regex in the ci-operator config."

This is misleading for test/ — if someone adds a new directory that also contains E2E tests (like a hypothetical test-extended/), they should NOT add it to the skip regex, but the error message implies they should. There's no documentation of the exclusion logic.

Suggested fix: Add a comment in toplevel-dirs.txt above test (or in a separate excluded-from-skip.txt section) and update the error message to reference the distinction explicitly:

# Directories listed here are tracked top-level dirs.
# NOT all of them belong in the pipeline_skip_if_only_changed regex.
# Dirs containing E2E tests (e.g. test/) must be excluded from the regex.
# See hack/ci/verify-toplevel-dirs.sh and CNTRLPLANE-3642 for context.

@mgencur mgencur Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. Let me update the text.

@mgencur mgencur Jun 19, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Hopefully.

token-minter
vendor
34 changes: 34 additions & 0 deletions hack/ci/verify-toplevel-dirs.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
#!/usr/bin/env bash
# Verifies that the tracked top-level directory list stays in sync with
# the actual git-tracked directories. This prevents the CI skip pattern
# (pipeline_skip_if_only_changed) from silently going stale when new
# top-level directories are added to the repo.
# Directories containing E2E tests directly or indirectly (e.g. test/)
# must be excluded from the regex.
set -euo pipefail

SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
TRACKED="${SCRIPT_DIR}/toplevel-dirs.txt"

# Collect top-level directories from both committed (HEAD) and staged
# (index) state, excluding hidden dirs. In CI the PR is already merged
# into HEAD; the ls-files fallback catches locally staged-but-uncommitted
# directories during local `make verify`.
ACTUAL=$( (git ls-tree -d --name-only HEAD 2>/dev/null; \
git ls-files --cached | grep '/' | cut -d/ -f1) \
| grep -v '^\.' | sort -u)
EXPECTED=$(sort "${TRACKED}")

DIFF=$(diff <(echo "${EXPECTED}") <(echo "${ACTUAL}") || true)

if [[ -n "${DIFF}" ]]; then
echo "ERROR: Top-level directory list is out of sync."
echo ""
echo "Diff (expected vs actual):"
echo "${DIFF}"
echo ""
echo "If you added or removed a top-level directory, update hack/ci/toplevel-dirs.txt."
echo "Also update the pipeline_skip_if_only_changed regex in the ci-operator config."
echo "Top-level directories containing E2E tests directly or indirectly (e.g. test/) must be excluded from the regex."
exit 1
fi