-
Notifications
You must be signed in to change notification settings - Fork 51
feat(bazel): guard the nested-module inventory #484
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,136 @@ | ||
| #!/usr/bin/env bash | ||
| # SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Guard the nested-module inventory during the Bazel consolidation. | ||
| # | ||
| # Every nested MODULE.bazel must be classified in tools/ci/nested-modules.txt. | ||
| # The check fails in both directions: | ||
| # | ||
| # unlisted module A nested MODULE.bazel exists that the ledger does not | ||
| # mention. Usually this means a new service arrived with its | ||
| # own module, which is the thing consolidation is trying to | ||
| # stop. The fix is normally to build from the root module, | ||
| # not to add a ledger line. | ||
| # | ||
| # stale entry The ledger names a module that no longer exists. This is | ||
| # what a completed migration looks like, and the entry has | ||
| # to go, or the file drifts into describing a repository | ||
| # that no longer exists. | ||
| # | ||
| # Why a ledger and not one allowlist: the plan separates vendored paths, | ||
| # not-yet-migrated modules, and permanent exceptions, because only the last | ||
| # carries an exception contract. Collapsing them would make every outstanding | ||
| # migration look like an accepted exception. | ||
| # | ||
| # Usage: check-nested-modules [--root DIR] [--ledger FILE] | ||
| set -euo pipefail | ||
|
|
||
| root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" | ||
| ledger="" | ||
|
|
||
| while [ "$#" -gt 0 ]; do | ||
| case "$1" in | ||
| --root) root="$2"; shift 2 ;; | ||
| --ledger) ledger="$2"; shift 2 ;; | ||
| *) echo "unknown argument: $1" >&2; exit 2 ;; | ||
| esac | ||
| done | ||
| [ -n "${ledger}" ] || ledger="${root}/tools/ci/nested-modules.txt" | ||
|
|
||
| if [ ! -f "${ledger}" ]; then | ||
| echo "ledger not found: ${ledger}" >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| # Declared paths, by category. | ||
| declared_all="$(mktemp)"; declared_ledger="$(mktemp)" | ||
| declared_exception="$(mktemp)"; found="$(mktemp)" | ||
| trap 'rm -f "${declared_all}" "${declared_ledger}" "${declared_exception}" "${found}" "${found}.raw"' EXIT | ||
|
|
||
| lineno=0 | ||
| while read -r category path _rest; do | ||
| lineno=$((lineno + 1)) | ||
| case "${category}" in | ||
| ''|\#*) continue ;; | ||
| esac | ||
| # A row naming a category but no path is a malformed ledger, not an empty | ||
| # one. Skipping it would let a typo silently shrink the guard's coverage. | ||
| if [ -z "${path}" ]; then | ||
| echo "${ledger}:${lineno}: category '${category}' with no path" >&2 | ||
| exit 2 | ||
| fi | ||
| case "${category}" in | ||
| vendored|scaffolding) ;; | ||
| ledger) printf '%s\n' "${path}" >> "${declared_ledger}" ;; | ||
| exception) printf '%s\n' "${path}" >> "${declared_exception}" ;; | ||
| *) | ||
| echo "${ledger}:${lineno}: unknown category '${category}' for ${path}" >&2 | ||
| exit 2 | ||
| ;; | ||
| esac | ||
| printf '%s\n' "${path}" >> "${declared_all}" | ||
| done < "${ledger}" | ||
|
|
||
| # One path, one category. Without this a repeated `ledger svc/a` inflates the | ||
| # backlog count, and the same path classified both `ledger` and `exception` | ||
| # inflates both while still passing, which would misreport how much of the | ||
| # migration is actually outstanding. | ||
| dupes="$(sort "${declared_all}" | uniq -d)" | ||
| if [ -n "${dupes}" ]; then | ||
| { | ||
| echo "error: ${ledger#"${root}"/} classifies the same path more than once:" | ||
| printf ' %s\n' ${dupes} | ||
| } >&2 | ||
| exit 2 | ||
| fi | ||
|
|
||
| sort -u -o "${declared_all}" "${declared_all}" | ||
|
|
||
| # Actual nested modules. The root module is the destination, not a nested one. | ||
| # Discovery failures must terminate: an unreadable tree that yields a partial | ||
| # inventory would quietly narrow what the guard can see, so `.git` is pruned | ||
| # explicitly rather than suppressing find's errors. | ||
| if ! ( cd "${root}" && find . -name .git -prune -o \ | ||
| -name MODULE.bazel ! -path './MODULE.bazel' -printf '%h\n' ) > "${found}.raw"; then | ||
| echo "error: failed to enumerate nested modules under ${root}" >&2 | ||
| exit 2 | ||
| fi | ||
| sed 's|^\./||' "${found}.raw" | sort -u > "${found}" | ||
|
|
||
| fail=0 | ||
|
|
||
| unlisted="$(comm -23 "${found}" "${declared_all}")" | ||
| if [ -n "${unlisted}" ]; then | ||
| fail=1 | ||
| { | ||
| echo "error: nested MODULE.bazel not classified in ${ledger#"${root}"/}:" | ||
| printf ' %s\n' ${unlisted} | ||
| echo | ||
| echo "A new nested module works against the consolidation. Prefer building" | ||
| echo "from the root module. If it genuinely cannot, add it under the" | ||
| echo "category that describes it, and for 'exception' record the owner," | ||
| echo "the rationale, and the residual risk." | ||
| } >&2 | ||
| fi | ||
|
|
||
| stale="$(comm -13 "${found}" "${declared_all}")" | ||
| if [ -n "${stale}" ]; then | ||
| fail=1 | ||
| { | ||
| echo "error: ${ledger#"${root}"/} lists modules that no longer exist:" | ||
| printf ' %s\n' ${stale} | ||
| echo | ||
| echo "If these migrated, delete their lines. The ledger is the migration" | ||
| echo "backlog, so a stale entry overstates how much work is left." | ||
| } >&2 | ||
| fi | ||
|
|
||
| [ "${fail}" -eq 0 ] || exit 1 | ||
|
|
||
| remaining=$(wc -l < "${declared_ledger}" 2>/dev/null || echo 0) | ||
| exceptions=$(wc -l < "${declared_exception}" 2>/dev/null || echo 0) | ||
| total=$(wc -l < "${found}") | ||
| echo "nested modules: ${total} classified" | ||
| echo " awaiting migration: ${remaining}" | ||
| echo " permanent exceptions: ${exceptions}" | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| # SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Every nested MODULE.bazel in the repository, classified. tools/ci/check-nested-modules | ||
| # fails if a nested module exists that is not listed here, and fails if an entry | ||
| # here no longer exists. Both directions matter: the first stops new nested | ||
| # modules appearing by accident, the second stops this file drifting into | ||
| # fiction as migration proceeds. | ||
| # | ||
| # Format: <category> <path-to-directory-containing-MODULE.bazel> | ||
| # | ||
| # Categories, in the sense docs/dev/bazel-consolidation.md gives them: | ||
| # | ||
| # vendored Third-party source vendored into the tree. Never in scope for | ||
| # consolidation, excluded from the count outright, and carries no | ||
| # exception contract because it is not ours to converge. | ||
| # | ||
| # scaffolding Exists to support the migration and is deleted when its | ||
| # consumers no longer need it. Not a service, so it is not | ||
| # counted as a module that must converge. | ||
| # | ||
| # ledger A first-party service module that has not migrated yet. This is | ||
| # the migration backlog. Entries leave this file entirely when the | ||
| # service moves to the root module. An entry here is a statement | ||
| # that the work is outstanding, not that it is excused. | ||
| # | ||
| # exception A first-party service that will keep its own module | ||
| # permanently. This is the only category that carries the | ||
| # exception contract in the plan: a recorded owner, a rationale, | ||
| # and the residual correctness risk in writing. Adding an entry | ||
| # here reduces the number of modules the end state converges, so | ||
| # it is a deliberate scope change, not a way to silence this | ||
| # check. | ||
|
|
||
| vendored src/compute-plane-services/nvca/vendor/cel.dev/expr | ||
|
|
||
| scaffolding rules/oci-destinations | ||
|
|
||
| ledger src/compute-plane-services/byoo-otel-collector | ||
| ledger src/compute-plane-services/ess-agent | ||
| ledger src/compute-plane-services/image-credential-helper | ||
| ledger src/compute-plane-services/nvca | ||
| ledger src/compute-plane-services/nvcf-unbound | ||
| ledger src/compute-plane-services/worker-init | ||
| ledger src/compute-plane-services/worker-llm-credentials | ||
| ledger src/compute-plane-services/worker-task | ||
| ledger src/compute-plane-services/worker-utils | ||
| ledger src/control-plane-services/function-autoscaler | ||
| ledger src/control-plane-services/helm-reval | ||
| ledger src/control-plane-services/nats-auth-callout | ||
| ledger src/invocation-plane-services/grpc-proxy | ||
| ledger src/invocation-plane-services/http-invocation | ||
| ledger src/invocation-plane-services/llm-api-gateway | ||
| ledger src/invocation-plane-services/ratelimiter | ||
| ledger src/invocation-plane-services/vanity-gateway | ||
| ledger src/libraries/rust/stargate | ||
|
|
||
| # No permanent exceptions today. nvsnap was the open question in Phase 1 and it | ||
| # migrated in NVIDIA/nvcf#471, so it is absent from this file entirely, which is | ||
| # what a completed migration looks like here. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,158 @@ | ||
| #!/usr/bin/env bash | ||
| # SPDX-FileCopyrightText: Copyright (c) NVIDIA CORPORATION & AFFILIATES. All rights reserved. | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
| # | ||
| # Behavioral test for tools/ci/check-nested-modules. | ||
| # | ||
| # A guard that only ever passes is indistinguishable from no guard, so assert | ||
| # the failures directly: an unlisted nested module must fail, and so must a | ||
| # ledger entry whose module is gone. Each case builds a throwaway tree rather | ||
| # than depending on the real repository, so these stay true as the real ledger | ||
| # shrinks. | ||
| set -euo pipefail | ||
|
|
||
| script="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/check-nested-modules" | ||
| fail=0 | ||
|
|
||
| # Build a fake repo: root MODULE.bazel plus a nested one per argument. | ||
| make_tree() { | ||
| local dir; dir="$(mktemp -d)" | ||
| touch "${dir}/MODULE.bazel" | ||
| local p | ||
| for p in "$@"; do | ||
| mkdir -p "${dir}/${p}" | ||
| touch "${dir}/${p}/MODULE.bazel" | ||
| done | ||
| printf '%s' "${dir}" | ||
| } | ||
|
|
||
| run() { # run <root> <ledger-contents> -> sets $out and $rc | ||
| local root="$1" contents="$2" ledger | ||
| ledger="$(mktemp)" | ||
| printf '%s\n' "${contents}" > "${ledger}" | ||
| set +e | ||
| out="$(bash "${script}" --root "${root}" --ledger "${ledger}" 2>&1)" | ||
| rc=$? | ||
| set -e | ||
| rm -f "${ledger}" | ||
| } | ||
|
|
||
| expect() { # expect <desc> <want-rc> [substring] | ||
| local desc="$1" want="$2" substr="${3:-}" | ||
| if [ "${rc}" != "${want}" ]; then | ||
| printf 'FAIL %s: want exit %s, got %s\n%s\n' "${desc}" "${want}" "${rc}" "${out}" | ||
| fail=1 | ||
| return | ||
| fi | ||
| if [ -n "${substr}" ] && ! printf '%s' "${out}" | grep -q -- "${substr}"; then | ||
| printf 'FAIL %s: output missing %q\n%s\n' "${desc}" "${substr}" "${out}" | ||
| fail=1 | ||
| return | ||
| fi | ||
| printf 'ok %s\n' "${desc}" | ||
| } | ||
|
|
||
| # Fully classified tree passes, and the root module is not counted as nested. | ||
| t="$(make_tree svc/a svc/b vendor/x)" | ||
| run "${t}" 'ledger svc/a | ||
| ledger svc/b | ||
| vendored vendor/x' | ||
| expect "classified tree passes" 0 "nested modules: 3 classified" | ||
| expect "counts only unmigrated as remaining" 0 "awaiting migration: 2" | ||
| rm -rf "${t}" | ||
|
|
||
| # A new nested module that nobody declared must fail and be named. | ||
| t="$(make_tree svc/a svc/sneaky)" | ||
| run "${t}" 'ledger svc/a' | ||
| expect "unlisted nested module fails" 1 "svc/sneaky" | ||
| rm -rf "${t}" | ||
|
|
||
| # A ledger entry whose module migrated must fail, so the backlog stays honest. | ||
| t="$(make_tree svc/a)" | ||
| run "${t}" 'ledger svc/a | ||
| ledger svc/already-migrated' | ||
| expect "stale ledger entry fails" 1 "svc/already-migrated" | ||
| rm -rf "${t}" | ||
|
|
||
| # Both directions at once are both reported, not just the first. | ||
| t="$(make_tree svc/a svc/new)" | ||
| run "${t}" 'ledger svc/a | ||
| ledger svc/gone' | ||
| expect "reports unlisted and stale together" 1 "svc/new" | ||
| expect " ... and names the stale one too" 1 "svc/gone" | ||
| rm -rf "${t}" | ||
|
|
||
| # Exceptions are classified but counted separately from the backlog. | ||
| t="$(make_tree svc/a svc/keeps-its-own)" | ||
| run "${t}" 'ledger svc/a | ||
| exception svc/keeps-its-own' | ||
| expect "exception passes the guard" 0 "permanent exceptions: 1" | ||
| expect "exception is not counted as backlog" 0 "awaiting migration: 1" | ||
| rm -rf "${t}" | ||
|
|
||
| # Comments and blank lines are ignored rather than parsed as entries. | ||
| t="$(make_tree svc/a)" | ||
| run "${t}" '# a comment | ||
|
|
||
| ledger svc/a | ||
| ' | ||
| expect "comments and blanks ignored" 0 "nested modules: 1 classified" | ||
| rm -rf "${t}" | ||
|
|
||
| # A typo in the category must be loud, not silently treated as classified. | ||
| t="$(make_tree svc/a)" | ||
| run "${t}" 'ledgar svc/a' | ||
| expect "unknown category is an error" 2 "unknown category" | ||
| rm -rf "${t}" | ||
|
|
||
| # A category with no path is a malformed ledger, not an empty one. Skipping it | ||
| # would silently narrow what the guard covers. | ||
| t="$(make_tree svc/a)" | ||
| run "${t}" 'ledger svc/a | ||
| ledger' | ||
| expect "category with no path is an error" 2 "with no path" | ||
| rm -rf "${t}" | ||
|
|
||
| # One path, one category. A repeat inflates the backlog count. | ||
| t="$(make_tree svc/a)" | ||
| run "${t}" 'ledger svc/a | ||
| ledger svc/a' | ||
| expect "duplicate path is an error" 2 "more than once" | ||
| rm -rf "${t}" | ||
|
|
||
| # The same path in two categories would inflate both counts while passing. | ||
| t="$(make_tree svc/a)" | ||
| run "${t}" 'ledger svc/a | ||
| exception svc/a' | ||
| expect "conflicting categories are an error" 2 "more than once" | ||
| rm -rf "${t}" | ||
|
|
||
| # Discovery must fail loudly. A root that does not exist cannot yield a | ||
| # trustworthy inventory, so it must not be reported as an empty one. | ||
| empty_ledger="$(mktemp)" | ||
| set +e | ||
| out="$(bash "${script}" --root /nonexistent-root-xyz --ledger "${empty_ledger}" 2>&1)"; rc=$? | ||
|
balajinvda marked this conversation as resolved.
|
||
| set -e | ||
| rm -f "${empty_ledger}" | ||
| expect "unreadable root is an error" 2 "failed to enumerate" | ||
|
|
||
| # A nested module inside .git must not be discovered (pruned, not suppressed). | ||
| t="$(make_tree svc/a)" | ||
| mkdir -p "${t}/.git/weird" && touch "${t}/.git/weird/MODULE.bazel" | ||
| run "${t}" 'ledger svc/a' | ||
| expect ".git contents are pruned" 0 "nested modules: 1 classified" | ||
| rm -rf "${t}" | ||
|
|
||
| # A missing ledger is a setup error, distinct from a policy violation. | ||
| t="$(make_tree svc/a)" | ||
| set +e | ||
| out="$(bash "${script}" --root "${t}" --ledger "${t}/nope.txt" 2>&1)"; rc=$? | ||
| set -e | ||
| expect "missing ledger is a setup error" 2 "ledger not found" | ||
| rm -rf "${t}" | ||
|
|
||
| if [ "${fail}" -ne 0 ]; then | ||
| echo "check-nested-modules: FAILED" >&2 | ||
| exit 1 | ||
| fi | ||
| echo "check-nested-modules: all checks passed" | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.