Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
74 changes: 22 additions & 52 deletions .githooks/pre-commit
Original file line number Diff line number Diff line change
@@ -1,26 +1,28 @@
#!/bin/sh
#
# Guards the App Configuration exports and the catalog that describes them.
# Validates the App Configuration catalog:
#
# Config/*.json scanned for credentials
# app-config.yaml validated against its own embedded schema
# app-config.yaml checked against its own embedded schema
#
# The per-environment exports it describes used to be scanned here too. LEGLINK-912 moved them
# to the private link-cac repository, so nothing under Config/ can be staged in this one any
# more. That repository has no hook of its own - scanning there would need a clone of this one
# on every machine - so its CI is what runs validate_aac_secrets.py against them.
#
# Enable for your clone with:
# git config core.hooksPath .githooks
#
# Validates the STAGED content, not the working tree, so a problem cannot slip through by
# being fixed in the file after `git add`.
#
# Bypass with `git commit --no-verify`. CI runs the same checks either way.
# Bypass with `git commit --no-verify`. CI runs the same check either way.

set -e

staged_exports=$(git diff --cached --name-only --diff-filter=ACM \
| grep -E '^Config/.*\.json$' || true)
staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \
| grep -E '^app-config\.yaml$' || true)

if [ -z "$staged_exports" ] && [ -z "$staged_catalog" ]; then
if [ -z "$staged_catalog" ]; then
exit 0
fi
Comment on lines 22 to 27

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.

🎯 Functional Correctness | 🟑 Minor | ⚑ Quick win

Reject a staged deletion of app-config.yaml.

--diff-filter=ACM excludes deletions. If the catalog is deleted from the index, staged_catalog is empty and Line 26 exits successfully. Detect the deletion and fail before the early return.

Proposed fix
+if git diff --cached --name-only --diff-filter=D -- app-config.yaml |
+   grep -q '^app-config\.yaml$'; then
+    echo "pre-commit: BLOCKED -- app-config.yaml cannot be deleted."
+    exit 1
+fi
+
 staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \
| grep -E '^app-config\.yaml$' || true)
if [ -z "$staged_exports" ] && [ -z "$staged_catalog" ]; then
if [ -z "$staged_catalog" ]; then
exit 0
fi
if git diff --cached --name-only --diff-filter=D -- app-config.yaml |
grep -q '^app-config\.yaml$'; then
echo "pre-commit: BLOCKED -- app-config.yaml cannot be deleted."
exit 1
fi
staged_catalog=$(git diff --cached --name-only --diff-filter=ACM \
| grep -E '^app-config\.yaml$' || true)
if [ -z "$staged_catalog" ]; then
exit 0
fi
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.githooks/pre-commit around lines 22 - 27, Update the staged-file detection
in the pre-commit hook so deletion of app-config.yaml is included, then check
for that deletion and reject it before the existing empty staged_catalog early
return. Preserve the current handling for added or modified app-config.yaml
files.


Expand All @@ -37,59 +39,27 @@ for candidate in python3 python py; do
done

if [ -z "$PYTHON" ]; then
echo "pre-commit: no working python found, skipping App Config checks."
echo "pre-commit: CI will still run them."
echo "pre-commit: no working python found, skipping the App Config catalog check."
echo "pre-commit: CI will still run it."
exit 0
fi

# Materialize staged blobs so the checks see exactly what is being committed.
# Materialize the staged blob so the check sees exactly what is being committed.
tmpdir=$(mktemp -d)
trap 'rm -rf "$tmpdir"' EXIT

stage_blob() {
dest="$tmpdir/$(basename "$1")"
git show ":$1" > "$dest"
echo "$dest"
}

if [ -n "$staged_exports" ]; then
files=""
for path in $staged_exports; do
files="$files $(stage_blob "$path")"
done

# --strict matches appconfig-secret-scan.yml, so the hook cannot pass something CI will
# reject. Errors block either way; --strict is only about warnings - a secret-shaped key
# holding a literal, a duplicate (key, label), a malformed entry. Those are worth stopping
# for here because these files go to a public repository, where a false positive costs a
# --no-verify and a false negative costs a rotated credential and permanent git history.
#
# It does not make the two identical: this scans the staged blob, CI scans all three files
# on disk. A warning in a file you did not stage still passes here and fails there.
# shellcheck disable=SC2086
if ! "$PYTHON" Scripts/AzureAppConfig/validate_aac_secrets.py --strict $files; then
echo ""
echo "pre-commit: BLOCKED -- staged App Config export failed the secret scan."
echo "pre-commit: An ERROR is a credential: move it into Key Vault and reference it,"
echo "pre-commit: then re-stage. A WARNING is something worth a look rather than"
echo "pre-commit: certainly wrong; it blocks here because CI runs --strict too."
echo "pre-commit: If the finding is wrong, commit with --no-verify."
exit 1
fi
fi
catalog="$tmpdir/$(basename "$staged_catalog")"
git show ":$staged_catalog" > "$catalog"

if [ -n "$staged_catalog" ]; then
catalog=$(stage_blob "$staged_catalog")
if ! "$PYTHON" Scripts/AzureAppConfig/validate_app_config_schema.py "$catalog"; then
echo ""
echo "pre-commit: BLOCKED -- staged app-config.yaml does not match its own schema."
echo "pre-commit: Fix the entries above, or commit with --no-verify."
exit 1
fi
if ! "$PYTHON" Scripts/AzureAppConfig/validate_app_config_schema.py "$catalog"; then
echo ""
echo "pre-commit: BLOCKED -- staged app-config.yaml does not match its own schema."
echo "pre-commit: Fix the entries above, or commit with --no-verify."
exit 1
fi

# The required-key check is deliberately NOT run here. It compares the catalog against all
# three stores, so it would fail on gaps a commit did not introduce and cannot fix. CI reports
# it instead.
# The required-key check is deliberately NOT run here. It compares the catalog against every
# store's export, which now means reading a second repository - and it would fail on gaps a
# commit did not introduce and cannot fix. Both repositories' CI report it instead.
Comment on lines +61 to +63

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.

πŸ“ Maintainability & Code Quality | 🟑 Minor | ⚑ Quick win

Correct the required-key CI ownership statement.

The public workflow now runs unit tests and schema validation only. The PR objective moves required-key checks to link-cac, but this comment says that both repositories report the check. State that link-cac CI reports it instead.

Proposed wording fix
-# commit did not introduce and cannot fix. Both repositories' CI report it instead.
+# commit did not introduce and cannot fix. The `link-cac` CI reports it instead.
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# The required-key check is deliberately NOT run here. It compares the catalog against every
# store's export, which now means reading a second repository - and it would fail on gaps a
# commit did not introduce and cannot fix. Both repositories' CI report it instead.
# The required-key check is deliberately NOT run here. It compares the catalog against every
# store's export, which now means reading a second repository - and it would fail on gaps a
# commit did not introduce and cannot fix. The `link-cac` CI reports it instead.
πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.githooks/pre-commit around lines 61 - 63, Update the explanatory comment
near the required-key check to state that link-cac CI reports the check,
replacing the inaccurate claim that both repositories' CI report it. Leave the
rationale about not running it in pre-commit unchanged.


exit 0
10 changes: 5 additions & 5 deletions .github/CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
**/pom.xml @lantanagroup/link-release-management-team
**/application*.yml @lantanagroup/link-release-management-team @lantanagroup/devops

# The configuration catalog and the per-environment App Configuration exports. These are what
# the App Config import pipeline deploys, so a change here changes deployed configuration.
# Note these are not covered by the patterns above: application*.yml matches neither the stem
# nor the extension of app-config.yaml.
# The configuration catalog: which keys exist, what they mean, and which are required. A change
# here is a change to what every environment must provision. Note it is not covered by the
# patterns above - application*.yml matches neither the stem nor the extension of
# app-config.yaml. The per-environment exports it catalogues moved to the private link-cac
# repository, which owns them under the same pattern in its own CODEOWNERS.
/app-config.yaml @lantanagroup/link-release-management-team @lantanagroup/devops
Config/app-config*.json @lantanagroup/link-release-management-team @lantanagroup/devops

docs/* @lantanagroup/devops @lantanagroup/link-arch

Expand Down
43 changes: 22 additions & 21 deletions .github/workflows/appconfig-catalog-check.yml
Original file line number Diff line number Diff line change
@@ -1,14 +1,26 @@
name: "App Config Catalog Check"

# Verifies app-config.yaml describes reality: every key marked required: true has a row in
# every environment store, no store row carries a label no service selects, and the Serilog
# sink index the catalog depends on is still pinned.
# Verifies app-config.yaml is internally sound: it conforms to the JSON Schema embedded in
# itself, and the rules deciding whether a catalog key is satisfied by a store row still hold.
#
# LEGLINK-775 will make a pipeline import Config/app-config.*.json into the stores, at which
# point a missing required key becomes a deployment defect rather than a documentation one.
# It does NOT check the catalog against the environment stores. That comparison needs the
# per-environment exports, which live in the private link-cac repository (LEGLINK-912), and
# this repository is public in both directions that matter:
#
# Runs on every PR rather than filtering on paths, so it stays valid as a required status
# check. A path filter would leave PRs that touch nothing here reporting no status at all.
# * Actions logs here are world-readable. check_required_config.py reports store rows that
# are absent from the catalog, and the labels those rows carry - private key names, even
# though it never prints a value.
# * A read token for link-cac sitting in this repository's secrets can be read by anyone with
# write access here, via a workflow change on a same-repo pull request.
#
# So link-cac is never read from here. It runs the required-key check itself, against a public
# checkout of this repository, on every pull request there plus daily - see
# .github/workflows/appconfig-checks.yml in that repository. The daily run is what catches a
# key marked required: true merged here with no rows added there.
#
# Runs on every PR rather than filtering on paths, so it can be made a required status check
# without further change. It is not one today. A path filter would leave PRs that touch nothing
# here reporting no status at all, which is what makes such a check unusable.
on:
pull_request:
branches:
Expand All @@ -26,14 +38,14 @@ on:
branches:
- dev

# This job only reads the repository: checkout, install PyYAML, then run the catalog checks.
# Nothing is written back, so the token needs no more than read access.
# This job only reads this repository: checkout, install PyYAML, then run the catalog checks.
# Nothing is written back and nothing else is read, so the token needs no more than read access.
permissions:
contents: read

jobs:
check:
name: Validate catalog and required keys
name: Validate catalog schema and rules
runs-on: ubuntu-latest
steps:
- name: Check out repository
Expand All @@ -44,7 +56,6 @@ jobs:
with:
python-version: '3.11'

# validate_aac_secrets.py is stdlib-only; these two need a YAML parser.
- name: Install dependencies
run: pip install "pyyaml==6.0.*"

Expand All @@ -53,13 +64,3 @@ jobs:

- name: Validate catalog schema
run: python Scripts/AzureAppConfig/validate_app_config_schema.py

# Enforcing. The gaps this was advisory for are closed: the three required keys now have
# rows in every environment file and the orphaned "Automation" label is gone.
#
# A failure here means app-config.yaml and Config/app-config.*.json have drifted - most
# often a key marked required: true in the catalog with no row added to each environment
# file. Fix it in the same PR: either add the row, or set required: false and record the
# shipped default in defaultValue.
- name: Check required keys are provisioned
run: python Scripts/AzureAppConfig/check_required_config.py
44 changes: 0 additions & 44 deletions .github/workflows/appconfig-secret-scan.yml

This file was deleted.

7 changes: 6 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -422,4 +422,9 @@ FodyWeavers.xsd
# dotnet run --file Scripts/AzureAppConfig/dump_config_symbols.cs -- DotNet Scripts/AzureAppConfig/config_symbols.json
# python Scripts/AzureAppConfig/extract_config_keys.py
Scripts/AzureAppConfig/config_symbols.json
Config/config-key-inventory.json
Scripts/AzureAppConfig/config-key-inventory.json

# Where the App Config workflows check out the private link-cac repository for its exports.
# Also the conventional place to put a local clone, though the tooling defaults to a sibling
# ../link-cac instead.
.link-cac/
Loading
Loading