Skip to content

validate-org: do not enforce protected owners before the org is onboarded - #71

Closed
dev-milos wants to merge 1 commit into
mainfrom
fix/validate-org-unmanaged
Closed

validate-org: do not enforce protected owners before the org is onboarded#71
dev-milos wants to merge 1 commit into
mainfrom
fix/validate-org-unmanaged

Conversation

@dev-milos

Copy link
Copy Markdown
Collaborator

Closes G-Research/gr-oss#1428

With protected_owners configured and no organisation config in the repository yet,
validate-org rejects every pull request:

No organisation teams.yaml or members.yaml found, validating as an empty organisation config
...
protected owner "<login>" is missing from members.yaml: protected identities cannot be removed

Nothing is being removed — the organisation has never been onboarded.

Why it blocks adoption

The guard is meant to be set before the bootstrap import, so that the import pull request
itself is checked. The bootstrap pull request does pass, because its staged members.yaml
lists the protected owners. But between setting the variable and merging that import,
every unrelated pull request is rejected — repository config changes have nothing to do
with organisation membership, yet they cannot get past the gate.

Cause

Treating an absent members.yaml as an empty member list is right for an organisation
already under management, where an empty list plans a member wipe. Before adoption it is
not: with no organisation config anywhere, Terraform creates no github_membership
resources, so there is no state a plan could remove.

runValidateOrg already computed and printed the distinguishing condition without acting
on it.

Change

State Before After
teams.yaml and members.yaml both absent rejected skipped, with a warning
members.yaml absent, teams.yaml present rejected rejected — a deleted file
members.yaml present, owner omitted rejected rejected

Enforcement resumes as soon as either file exists, including staged bootstrap output, so
the highest-stakes plan is still checked. The command's help text stated the rule without
this exception and has been updated.

Tests

Two added, and the existing coverage is what proves the guard was not weakened:

  • UnmanagedOrgDoesNotEnforceProtectedOwners — no config, owners set, passes with a warning
  • StagedBootstrapEnforcesProtectedOwners — staged output omitting an owner is still rejected
  • DeletedMembersFileStillEnforcesProtectedOwners and EmptyMembersListEnforcesProtectedOwners
    continue to pass unchanged

Verification

  • go build, go vet, go test ./... pass
  • run against a real config repository with no organisation config: fails before, passes
    after; adding a teams.yaml makes it fail again, as intended

…rded

With protected owners configured and no organisation config in the repository yet,
every pull request was rejected for removing owners that were never there.

Treating an absent members.yaml as an empty member list is right for an organisation
already under management, where an empty list plans a member wipe. Before adoption it
is not: with no organisation config anywhere, Terraform creates no membership
resources, so there is no state a plan could remove.

runValidateOrg already computed and printed the distinguishing condition without
acting on it. Enforcement is now skipped only when teams.yaml and members.yaml are
both absent from the promoted and the staged location, and the run says so. It
resumes as soon as either file exists, including staged bootstrap output, so the
import that first takes ownership of the organisation is still checked.
@dev-milos

Copy link
Copy Markdown
Collaborator Author

Closing unmerged — the exemption reopens the hole the guard exists to close.

The predicate that would make it safe is "Terraform state contains no github_membership
resources", and that is not derivable from the checked-out tree. A tree with neither
organisation/teams.yaml nor organisation/members.yaml has two origins: never onboarded,
and a pull request that deletes both. This change grants the exemption to both.

Verified against this branch:

Case main this branch
owner omitted from an existing members.yaml rejected rejected
pull request deletes both files rejected passes

members.tf falls back to members: [] when the file is absent, so for an onboarded
organisation the deleted-files tree plans a destroy of every membership. That is the
highest-consequence form of the removal, and it is exactly what the guard is for.

The reasoning in the description — "Terraform creates no membership resources from that
state, so there is nothing a plan could remove" — is wrong. The absence of config is what
causes the wipe, not evidence that there is nothing to wipe.

The original complaint stands but is not worth weakening a permanent control for: the
window where unrelated pull requests are blocked is one-time, short, and under the
operator's control. The correct operating order is to run the bootstrap import first and
set protected_owners afterwards.

@dev-milos dev-milos closed this Aug 11, 2026
@dev-milos
dev-milos deleted the fix/validate-org-unmanaged branch August 11, 2026 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant