Extend registry checks to the development registry - #9233
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2fc23d6a-ef10-4f60-b369-2770f0af80d6
|
Azure Pipelines: 22 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: 22 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Extends extension registry approval checks and ownership rules to the development registry.
Changes:
- Detects and validates changes to either registry, including renames.
- Supports registry-only updates affecting one or both registries.
- Adds equivalent CODEOWNERS coverage and tests.
Show a summary per file
| File | Description |
|---|---|
.github/workflows/ext-registry-check.yml |
Detects changes to both registries. |
.github/scripts/src/ext-registry-check.js |
Applies policy checks independently per registry. |
.github/scripts/test/ext-registry-check.test.js |
Tests development and dual-registry updates. |
.github/CODEOWNERS |
Assigns development registry owners. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Medium
richardpark-msft
left a comment
There was a problem hiding this comment.
Small stuff - generally, I dislike default values as they usually just lead to weird bugs. There's no compat requirements for that function, so change the signature at will :)
richardpark-msft
left a comment
There was a problem hiding this comment.
Sorry, meant to use request changes earlier.
jongio
left a comment
There was a problem hiding this comment.
The per-registry loop and dual-path detection look correct, and the tests cover the dev-only and both-registries cases. One follow-on tied to the existing request to drop the registryPath defaults: see the inline note on DEFAULT_REGISTRY_JSON_PATH.
Remove implicit registry path and base ref defaults, and include the supported registry paths in validation errors.
jongio
left a comment
There was a problem hiding this comment.
Confirmed the removed defaults are safe: run() passes registryPath and registryBaseRef explicitly at every call site, and falls back to base sha then 'main' before invoking the check. My earlier note about the dangling DEFAULT_REGISTRY_JSON_PATH constant is resolved.
|
/check-enforcer evaluate |
richardpark-msft
left a comment
There was a problem hiding this comment.
Thanks for addressing the feedback. Now...you just have copilot to deal with.
jongio
left a comment
There was a problem hiding this comment.
Re-reviewed against 40743ce. The multi-registry refactor is correct: the allowed-update check runs per changed registry, diffChangedFiles flags non-registry edits and registry renames for core review, and the workflow pre-check mirrors the script including the rename (previous_filename) case. Tests cover dev-only, both-registries, rename, and independent per-registry evaluation. My earlier note about the unused default path was addressed.
Co-authored-by: tg-msft <tg-msft@users.noreply.github.com>
|
/check-enforcer override |
jongio
left a comment
There was a problem hiding this comment.
Two things, neither blocking.
ext-registry-ci.yml still only triggers on registry.json, so a dev-registry-only PR gets no schema or dependency validation at all. cli-ci.yml excludes cli/azd/extensions/**, and the validation step runs -run '^TestRegistryFileIsValid$', which doesn't match TestDevRegistryFileIsValid. That test is already written against registry.dev.json but nothing in PR CI fires it.
This matters more after this PR: registry.dev.json moves off the /** catch-all (6 owners) onto the registry maintainer list (17 owners), and it can now take the no-core-review fast path. So the set of people who can land a dev registry change grows while the validation gate stays prod-only.
Two edits in ext-registry-ci.yml:
paths:
- "cli/azd/extensions/registry.json"
- "cli/azd/extensions/registry.dev.json"
- ".github/workflows/ext-registry-ci.yml"go test ./pkg/extensions -run '^(TestRegistryFileIsValid|TestDevRegistryFileIsValid)$' -vverify-ext-providers.yml already uses that multi-test regex form, so it matches what's there.
Happy to see this as a follow-up rather than in this PR, but the two gates drifting apart is the kind of thing that stays broken quietly.
Deleting a registry previously fell through to the registry fetch and surfaced as "Internal failure in script: Not Found". Report it as a review reason instead, and skip removed paths when loading registries. Match registry files on either side of a rename so a registry renamed away is reported as a rename rather than an unrelated outside file. Correct the workflow comment, since previous_filename is only set on renames and deletions are already covered by the filename check. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 69dcf535-a40a-4d94-8fc3-9f9bf62a03e2
jongio
left a comment
There was a problem hiding this comment.
Re-reviewed against 177da8e. The deletion and rename-away handling is correct. Removed registry paths are excluded from the fetch loop, so a deleted registry surfaces as a policy reason instead of the old "Not Found" internal error, and isRegistryFile classifies a registry renamed away as a rename rather than an unrelated outside file. The added tests cover both cases with negative assertions that getContent isn't called and no internal-failure message is emitted. The workflow comment now matches actual behavior: previous_filename is only set on renames, and deletions stay covered by the filename check.
jongio
left a comment
There was a problem hiding this comment.
Re-confirmed against 177da8e. The per-registry evaluation, rename, and deletion paths all behave as intended and are exercised by the added tests. Deleted or renamed-away registries surface as policy reasons instead of hitting the content fetch, and the workflow's registry_changed output rename is applied consistently across both gate conditions. Nothing changed since my last review on this commit. Approving.
jongio
left a comment
There was a problem hiding this comment.
Reviewed the latest revision end to end. Each registry is evaluated independently, deleted and renamed registries route to core review without attempting a head fetch, and the check still fails closed on any internal error. Dev-registry, both-registry, rename-in, rename-away, and delete paths are all covered by tests. Nothing blocking from me.
jongio
left a comment
There was a problem hiding this comment.
Two hardening items on the registry path handling, both non-blocking.
Low:
- The workflow's inline registry path list can drift from
REGISTRY_JSON_PATHSin the script, and one drift direction silently skips the check. - A registry path that is absent at the base ref still ends in
Internal failure in scriptrather than a policy message.
jongio
left a comment
There was a problem hiding this comment.
Both comments I left are non-blocking, so this is fine to merge as is.
jongio
left a comment
There was a problem hiding this comment.
One gap left over from this change, in a file outside the diff.
registry.dev.json now gets the same approval policy as registry.json, but not the same validation CI. .github/workflows/ext-registry-ci.yml still triggers only on cli/azd/extensions/registry.json, and cli-ci.yml explicitly excludes cli/azd/extensions/**. So a PR touching only registry.dev.json runs neither workflow, and TestDevRegistryFileIsValid never executes on it. That's exactly the PR shape this change is meant to enable.
The failure mode isn't a red check on the registry PR, it's a green one. A malformed or dependency-inconsistent registry.dev.json merges clean, then breaks the next unrelated cli/azd PR that does trigger cli-ci.
Adding the path to the trigger closes half of it:
paths:
- "cli/azd/extensions/registry.json"
- "cli/azd/extensions/registry.dev.json"
- ".github/workflows/ext-registry-ci.yml"The other half is the test selector in that job, which is pinned to the prod test only:
go test ./pkg/extensions -run '^TestRegistryFileIsValid$|^TestDevRegistryFileIsValid$' -v
The step name and the ::error:: text there both say registry.json, so they'd need generalizing with it.
Happy for this to be a follow-up if you'd rather keep this PR scoped to the approval policy.
jongio
left a comment
There was a problem hiding this comment.
All three items I have open here are non-blocking: the duplicated registry path list in the workflow, the base-ref-absent case that still ends in Internal failure in script, and TestDevRegistryFileIsValid never running because ext-registry-ci.yml isn't triggered by registry.dev.json. Worth handling separately, and the CI trigger one is outside the scope of #9234 anyway.
I re-ran the script suite against 177da8e (56 pass) and exercised the delete, rename-away, rename-into, and added-at-head paths directly against this branch. Deletions and renames report policy messages now instead of a script failure, and each registry is evaluated independently.
|
/check-enforcer override |
|
@jongio thanks, confirmed the gap. Filed as #9337 and keeping this PR scoped to the approval policy. I verified it end to end: Two small corrections captured in the issue:
One thing I added to the issue while looking: |
Closes #9234
This PR extends the existing extension registry approval policy to
cli/azd/extensions/registry.dev.json, applying the same validation and CODEOWNERS rules currently used for the production registry.Changes
ext-registry-checkwhen either production or development registry changes, including rename scenarios.registry.dev.jsonthe same CODEOWNERS asregistry.json.Testing
npm test: 54 tests passed, with 5 opt-in live tests skipped.RUN_LIVE_TESTS=1 npm test: all 59 tests passed, including the existing real GitHub PR scenarios.registry.dev.jsondisplay-name update passed.registry.dev.jsonmetadata change.