Skip to content

NO-ISSUE: Add one-time TF state rm support for unrefreshable resources - #162

Merged
eliorerz merged 4 commits into
osac-project:mainfrom
eliorerz:osac-1737-fix-apply-orphaned-members
Aug 5, 2026
Merged

NO-ISSUE: Add one-time TF state rm support for unrefreshable resources#162
eliorerz merged 4 commits into
osac-project:mainfrom
eliorerz:osac-1737-fix-apply-orphaned-members

Conversation

@eliorerz

@eliorerz eliorerz commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Terraform's scheduled/push-triggered apply has been failing atomically for several runs, always on the same error:

Error: Error
  with github_membership.all["mvaskima"] (and jvalimak, srasanen)
  member is an invalid value for argument [{{} role}]

These 3 users are no longer in members.csv, but Terraform's remote state still tracks their github_membership/github_team_membership resources. Refreshing their live state fails (they've left the org), and that failure aborts the whole apply before anything else in the plan lands — including unrelated, already-merged changes like #161's archived = true settings.

This adds a one-time, manual-only state_rm_addresses input to the Apply workflow, mirroring the existing import_address/import_id mechanism. It's a no-op for the normal scheduled/push triggers. Once merged, I'll dispatch it once with the 6 orphaned resource addresses to clear them, then a normal apply should go through cleanly.

Update: addressed two rounds of CodeRabbit review (verified each finding against actual behavior rather than taking them at face value — see inline thread replies for details):

  • queue: max on the concurrency group, so a manual one-time recovery run dispatched while a scheduled/push run is in progress can't get silently dropped and replaced before it executes (default queue behavior only keeps one pending run).
  • Reject state_rm_addresses containing a newline before parsing (a real gap — read would otherwise silently truncate at the first newline).
  • Simplified the concurrency group to a single github.workflow key (the original github.event.pull_request.number || github.ref was always effectively just github.ref, since this workflow has no pull_request trigger).
  • Dropped GITHUB_TOKEN from the state-removal step — confirmed tofu state rm never calls the GitHub provider API, only the state backend.
  • One finding ("read needs -r") didn't hold up against the actual committed code (read -ra already implies -r) — left as a reply rather than a no-op change.

Summary by CodeRabbit

  • New Features

    • Added an optional manual workflow input for removing specified infrastructure state entries.
    • Added validation to reject empty, malformed, or unsafe state addresses before processing.
  • Improvements

    • Workflow runs are now serialized globally, with pending runs retained in a queue to prevent conflicting operations.

@coderabbitai

coderabbitai Bot commented Aug 4, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

The apply workflow adds an optional state_rm_addresses input for manual runs. Manual runs validate and remove each listed Terraform state address before applying configuration. The workflow now serializes all runs globally and retains pending runs.

Changes

Terraform state management

Layer / File(s) Summary
Global workflow serialization
.github/workflows/apply.yaml
The workflow uses one concurrency group for all manual, scheduled, and push-triggered runs. Pending runs remain queued.
Manual state removal before apply
.github/workflows/apply.yaml
The workflow accepts comma-separated Terraform state addresses. It rejects newline-separated, empty, and option-like addresses before running tofu state rm during manual dispatches. Automatic triggers skip the step.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • osac-project/github-config#169: Both PRs modify .github/workflows/apply.yaml to validate comma-separated Terraform resource addresses, but this PR uses tofu state rm while that PR uses apply -exclude.

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors, 1 warning)

Check name Status Explanation Resolution
No-Injection-Vectors ❌ Error The workflow interpolates workflow_dispatch import_address/import_id directly into a bash run block; a quote-and-command payload can execute shell commands. Pass import inputs through environment variables and use quoted shell variables, for example tofu import "$IMPORT_ADDRESS" "$IMPORT_ID".
No-Sensitive-Data-In-Logs ❌ Error The state-removal step logs raw resource addresses, both through tofu state rm and the invalid-address error; membership addresses contain GitHub usernames. Avoid interpolating ${addr} in errors and redact or suppress tofu state rm output when addresses can contain personal identifiers.
Ai-Attribution ⚠️ Warning The PR and two commits mention CodeRabbit, but all four PR commits lack Assisted-by or Generated-by trailers; no AI Co-Authored-By trailer was found. Add an Assisted-by or Generated-by trailer to each relevant PR commit, and do not use Co-Authored-By for CodeRabbit or other AI tools.
✅ Passed checks (8 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the one-time Terraform state removal support for unrefreshable resources.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
No-Hardcoded-Secrets ✅ Passed The added workflow lines contain no credential literals, private-key material, embedded credentials, or long blobs; AWS, app, and GitHub credentials remain secret references.
No-Weak-Crypto ✅ Passed The PR diff only adds workflow input validation, state removal, and concurrency changes; searches found no weak algorithms, custom crypto, or non-constant-time secret comparisons.
Container-Privileges ✅ Passed The changed workflow has no privileged, hostPID, hostNetwork, hostIPC, SYS_ADMIN, allowPrivilegeEscalation, or container security settings; it runs on ubuntu-latest.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with 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.

Inline comments:
In @.github/workflows/apply.yaml:
- Around line 79-87: Update the workflow concurrency configuration and the
state-removal/apply flow so all manual, push, and scheduled runs share one
repository-wide concurrency key for the entire state boundary. Ensure no run can
execute the TF State Remove step concurrently with another run’s tofu apply or
state read, while preserving the existing manual state-removal behavior.
- Around line 82-85: Replace the per-address loop in the state-removal step with
one tofu state rm invocation receiving all parsed ADDRS elements as arguments.
Preserve the comma-separated input parsing via state_rm_addresses, but ensure
the shared state object is updated only once and failures do not leave partial
removals.
- Around line 82-87: Update the state-removal step around the ADDRS loop to pass
inputs.state_rm_addresses through the step environment, then read only the
quoted STATE_RM_ADDRESSES variable inside Bash. Remove the direct
workflow-expression interpolation from the shell source while preserving comma
splitting and tofu state rm execution for each address.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: e9dfe635-b5be-440b-92cd-7313e97f1328

📥 Commits

Reviewing files that changed from the base of the PR and between 16b9f21 and 95ef270.

📒 Files selected for processing (1)
  • .github/workflows/apply.yaml

Comment thread .github/workflows/apply.yaml Outdated
Comment thread .github/workflows/apply.yaml Outdated
Comment thread .github/workflows/apply.yaml Outdated
@eliorerz

eliorerz commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Reviewed for real (not a rubber-stamp) — can't formally approve my own PR via the API, so noting findings here before merging.

  • state_rm_addresses defaults to empty and is gated by if: inputs.state_rm_addresses != '', so it's a true no-op for the scheduled/push triggers — matches the existing import_address/import_id pattern exactly.
  • Placement (after TF Import, before TF Apply) is right — state has to be cleaned up before apply attempts to refresh it.
  • The comma-split loop is correct bash, and since GH Actions run: steps default to bash -eo pipefail, a bad address in the list fails the step loudly rather than silently skipping — the right failure mode for a manual, one-time operation.
  • Minor observation, non-blocking: this interpolates ${{ inputs.state_rm_addresses }} directly into the shell script rather than via env: + "$VAR". Not a new issue — it's exactly the same pattern the existing import_address/import_id step already uses one step above — so fixing only the new step would just make the file inconsistent. Since workflow_dispatch inputs require write access to trigger (not attacker-reachable via a fork PR), this doesn't need to block merging; worth a follow-up someday to harden both steps together.

This addresses a second, independent instance of the same structural gap as the archived-repo issue (host-management-openstack, #165): one un-refreshable/broken resource aborting the entire atomic apply and silently blocking every other pending change. Merging.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🤖 Prompt for all review comments with 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.

Inline comments:
In @.github/workflows/apply.yaml:
- Around line 92-93: Remove the unused GITHUB_TOKEN environment entry from the
tofu state-removal step, leaving the AWS backend credentials and all other step
configuration unchanged.
- Around line 90-91: Update the state removal parsing around ADDRS to reject
STATE_RM_ADDRESSES values containing newline characters, preserving the existing
comma-separated behavior, and change read to use -r so backslashes in OpenTofu
addresses are retained. Keep tofu state rm "${ADDRS[@]}" unchanged.
- Around line 28-36: Update the workflow’s concurrency configuration around the
group key `${{ github.workflow }}` by adding the supported queue setting with a
maximum pending-run limit, so newer push or scheduled runs do not replace a
pending manual recovery run. Preserve the single global concurrency group and
existing apply ordering.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 977ea332-e28f-4aef-aafc-e2d54f32bf75

📥 Commits

Reviewing files that changed from the base of the PR and between 95ef270 and e4a5c18.

📒 Files selected for processing (1)
  • .github/workflows/apply.yaml

Comment thread .github/workflows/apply.yaml
Comment thread .github/workflows/apply.yaml
Comment thread .github/workflows/apply.yaml Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with 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.

Inline comments:
In @.github/workflows/apply.yaml:
- Around line 98-101: Update the state removal flow in
.github/workflows/apply.yaml to validate each comma-separated token in
STATE_RM_ADDRESSES before invoking tofu state rm. Reject empty tokens and tokens
beginning with “-”, including values whose first token is option-like, while
preserving the existing single-line validation and allowing valid resource
addresses.
- Line 45: Update the actionlint enforcement in the workflow validation
configuration so version 1.7.12 does not reject the valid concurrency.queue key.
Prefer upgrading the validator to a version that supports this GitHub Actions
syntax; if that version is required, add a narrowly scoped suppression for this
check while continuing to validate the rest of the workflow.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: eb5ee7dc-f0ac-4e0a-968c-793eeb25cef7

📥 Commits

Reviewing files that changed from the base of the PR and between e4a5c18 and 3016cad.

📒 Files selected for processing (1)
  • .github/workflows/apply.yaml

Comment thread .github/workflows/apply.yaml
Comment thread .github/workflows/apply.yaml
eliorerz added a commit to eliorerz/github-config that referenced this pull request Aug 5, 2026
… creds

- Trim leading/trailing whitespace around each split address -- "addr1,
  addr2" (space after the comma) is the natural way to type this list
  by hand, and untrimmed input would silently pass a mismatching
  address to -exclude.
- Reject newline-separated input and empty/option-like (leading "-")
  tokens, mirroring the same hardening already applied to
  state_rm_addresses in osac-project#162.
- Explicitly clear AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY for the
  issue-filing step, which only needs GITHUB_TOKEN -- it was otherwise
  inheriting the job-level backend credentials unnecessarily.

Not switching to -exclude-file: comma-splitting is still ambiguous for
a resource address that itself contains a literal comma inside a
bracketed for_each key (e.g. module.x["a,b"]), but no address in this
repo's actual configuration does that today, and switching input
format is a bigger, more disruptive change than the realistic benefit
justifies for a manual, human-operated one-time escape hatch.

Signed-off-by: Elior Erez <eerez@redhat.com>
@eliorerz
eliorerz force-pushed the osac-1737-fix-apply-orphaned-members branch from bcf99ba to e5acfd6 Compare August 5, 2026 22:42
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

eliorerz added a commit to eliorerz/github-config that referenced this pull request Aug 5, 2026
A single broken/unrefreshable resource aborts the entire tofu apply
atomically, blocking every other repo's pending Terraform changes with
no signal beyond a red Actions run. This has happened twice: the
host-management-openstack archived-repo bug (osac-project#165) and the orphaned
github_membership entries for users who left the org (osac-project#162). The
CaaS-Netris required-check rename sat un-applied for hours because of
the former, discovered only by chance while investigating an unrelated
CI queue backlog.

Two changes:

- File (or comment on) a tracking issue when tofu apply fails, so a
  stuck apply is never silent again. Uses the same GitHub App token
  already generated for tofu itself (issues:write is already granted),
  no new secrets needed.

- Add a one-time, manual `exclude_addresses` workflow_dispatch input
  (mirrors osac-project#162's `state_rm_addresses` pattern) that threads into
  `tofu apply -exclude=...`, letting a maintainer immediately unblock
  every other repo's changes when one resource is known-broken, while
  a permanent fix is prepared -- without giving up single-pass,
  dependency-complete apply for the normal case.

A permanent per-module `-target` loop was considered and rejected:
several modules share cross-module resources (e.g.
github_team.all["wg-infra"], referenced by ruleset_bypass_team_ids in
5+ repo modules), so looping per module would redundantly re-plan/
re-apply those shared resources on every iteration referencing them,
and gives up Terraform's whole-graph dependency ordering for no real
isolation benefit in the common case where nothing is broken.

Signed-off-by: Elior Erez <eerez@redhat.com>
eliorerz added a commit to eliorerz/github-config that referenced this pull request Aug 5, 2026
… creds

- Trim leading/trailing whitespace around each split address -- "addr1,
  addr2" (space after the comma) is the natural way to type this list
  by hand, and untrimmed input would silently pass a mismatching
  address to -exclude.
- Reject newline-separated input and empty/option-like (leading "-")
  tokens, mirroring the same hardening already applied to
  state_rm_addresses in osac-project#162.
- Explicitly clear AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY for the
  issue-filing step, which only needs GITHUB_TOKEN -- it was otherwise
  inheriting the job-level backend credentials unnecessarily.

Not switching to -exclude-file: comma-splitting is still ambiguous for
a resource address that itself contains a literal comma inside a
bracketed for_each key (e.g. module.x["a,b"]), but no address in this
repo's actual configuration does that today, and switching input
format is a bigger, more disruptive change than the realistic benefit
justifies for a manual, human-operated one-time escape hatch.

Signed-off-by: Elior Erez <eerez@redhat.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with 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.

Inline comments:
In @.github/workflows/apply.yaml:
- Around line 102-109: Update the STATE_RM_ADDRESSES validation before the
IFS-based parsing in the apply workflow to reject leading commas, trailing
commas, and repeated commas. Ensure malformed values such as “module.foo,”,
“,module.foo”, and “module.foo,,module.bar” exit with the existing
invalid-address error before tofu state rm receives any addresses.
- Around line 28-45: Update the apply workflow before the TF Apply step to skip
queued automatic push or schedule runs that have been superseded by a newer
revision, while allowing manual recovery runs to execute. Preserve serialization
through the shared concurrency group for all runs, and ensure the supersession
check does not discard workflow_dispatch recovery operations.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 682d64a7-b9d3-47df-87fe-7ac6eea34fd5

📥 Commits

Reviewing files that changed from the base of the PR and between fca1abc and e5acfd6.

📒 Files selected for processing (1)
  • .github/workflows/apply.yaml

Comment on lines +28 to +45
# This workflow has no pull_request trigger, so github.event.pull_request.number
# is always null here -- the group key collapsed to just github.ref in
# practice, which only serializes runs against the same ref. A manual
# workflow_dispatch run (e.g. one-time state rm) dispatched against a
# non-default ref could then run concurrently with a scheduled/push apply
# against main, racing on the same remote state. Use a single, unqualified
# group so every run of this workflow -- manual or automatic, any ref --
# is always serialized against every other.
group: ${{ github.workflow }}
cancel-in-progress: false
# Default queue behavior only keeps the single most-recently-queued run
# pending in a group -- an older pending run gets canceled and replaced.
# A manual one-time recovery run (state rm, or the exclude_addresses
# escape hatch) dispatched while a scheduled/push run is in progress
# could get silently dropped and replaced by the next automatic trigger
# before it ever executes. queue: max keeps every pending run queued
# (up to GitHub's cap of 100) instead of dropping older ones.
queue: max

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- workflow files ---'
git ls-files '.github/workflows/*'
printf '%s\n' '--- apply.yaml structure ---'
wc -l .github/workflows/apply.yaml
sed -n '1,150p' .github/workflows/apply.yaml
printf '%s\n' '--- queue/concurrency references ---'
rg -n --hidden -S 'queue:|concurrency:|cancel-in-progress|workflow_dispatch|schedule:|push:' .github/workflows .github 2>/dev/null || true

Repository: osac-project/github-config

Length of output: 7096


🌐 Web query:

GitHub Actions concurrency workflow syntax queue max supported key pending runs group cancel-in-progress

💡 Result:

GitHub Actions provides the concurrency keyword to manage the execution of workflows and jobs within a shared concurrency group. By default, GitHub Actions ensures that only one job or workflow run can be active (in-progress) and at most one can be in a pending state within a concurrency group [1][2][3]. Key configuration options include: - group: A string or dynamic expression that defines the identifier for the concurrency group. Workflows or jobs sharing the same group name are subject to concurrency constraints [2][4]. - cancel-in-progress: A boolean or conditional expression. When set to true, any currently running job or workflow in the group will be canceled when a new run is triggered. By default (or when set to false), only pending jobs are canceled when a new job is queued [1][3]. - queue: This property enables advanced queuing for pending jobs. - single (default): Allows at most one pending job. If a new job is queued, any existing pending job is canceled and replaced by the new one [1][2]. - max: Allows up to 100 jobs to be queued in a pending state. When the queue reaches this limit, additional triggered jobs are canceled [1][5][6]. Important constraints: - Incompatibility: The queue: max property cannot be used in conjunction with cancel-in-progress: true because they define conflicting behaviors for managing concurrent runs [2][3]. - Queue Limit: When using queue: max, the concurrency group supports up to 100 queued runs; any runs triggered beyond this capacity will be rejected [6]. - Uniqueness: Concurrency group names are scoped to the repository. If multiple workflows share the same group name, they will all be subject to the same concurrency limits [4][3].

Citations:


🌐 Web query:

site:github.com/actions/checkout README ref SHA defaults triggering event checkout commit

💡 Result:

When using the GitHub Actions actions/checkout action, the ref input defaults to the specific reference (branch, tag) or commit SHA that triggered the workflow run [1][2]. If the action is checking out a repository other than the one that triggered the workflow, it defaults to the repository's default branch [1][2]. By default, the action fetches only a single commit (fetch-depth: 1) corresponding to that ref or SHA [1][3]. Note that for certain event types, such as pull_request, the workflow might be triggered by a specific commit, and the checkout action will default to that SHA in a detached HEAD state [1][4]. Users often explicitly set the ref to github.head_ref or github.event.pull_request.head.sha when working with pull request events to ensure the desired branch or commit is checked out correctly [1][5].

Citations:


🌐 Web query:

site:docs.github.com GitHub Actions concurrency ordering is not guaranteed queue order

💡 Result:

According to official GitHub Actions documentation, jobs or workflow runs within the same concurrency group are processed in a first-in-first-out (FIFO) order, specifically based on the time each run started waiting on the concurrency group rather than the time the workflow was originally dispatched [1][2]. However, GitHub explicitly notes that ordering is not guaranteed [1][3]. This is because the actual start time of a job or workflow run—which determines its position in the queue—can vary due to system factors, meaning the sequence in which runs enter the "pending" state may not always strictly mirror the intended execution order [1][2]. When using concurrency controls: - By default, only one pending run is allowed in a group; new runs will cancel existing pending runs [1][4]. - You can enable queuing by using the queue property (e.g., queue: max), which allows multiple runs to wait in the concurrency group [1][2]. - Concurrency group names are case-insensitive, and using the same name across different workflows will cause them to share the same concurrency queue [1][5].

Citations:


🏁 Script executed:

#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
from itertools import permutations
import re

path = Path(".github/workflows/apply.yaml")
text = path.read_text()

assert re.search(r"(?m)^  group:\s*\$\{\{\s*github\.workflow\s*\}\}\s*$", text)
assert re.search(r"(?m)^  cancel-in-progress:\s*false\s*$", text)
assert re.search(r"(?m)^  queue:\s*max\s*$", text)
assert re.search(r"(?m)^      - uses:\s*actions/checkout@[^ ]+\s*$", text)
assert re.search(r"(?m)^      - name:\s*TF Apply\s*$", text)

events = [
    ("push", "commit-A"),
    ("push", "commit-B"),
    ("push", "commit-C"),
]
# actions/checkout defaults to the triggering ref/SHA, so each queued run
# applies the commit attached to its event.
queued = [(event, sha) for event, sha in events]
orders = list(permutations(queued))
stale_order = next(order for order in orders if [sha for _, sha in order] !=
                   [sha for _, sha in queued])

print("shared concurrency group: Apply configuration")
print("pending-run capacity: 100")
print("checkout/apply revisions in trigger order:", [sha for _, sha in queued])
print("possible execution order:", [sha for _, sha in stale_order])
print("stale revision can execute after a newer revision:",
      stale_order.index(("push", "commit-A")) >
      stale_order.index(("push", "commit-B")))
PY

Repository: osac-project/github-config

Length of output: 436


🏁 Script executed:

#!/bin/bash
set -eu
python3 - <<'PY'
from itertools import permutations

revisions = ["commit-A", "commit-B", "commit-C"]
for order in permutations(revisions):
    if any(order.index(older) > order.index(newer)
           for older, newer in zip(revisions, revisions[1:])):
        print("trigger order:", revisions)
        print("possible execution order:", list(order))
        print("older revision executes after newer revision: true")
        break
else:
    raise SystemExit("No stale execution order found")
PY

Repository: osac-project/github-config

Length of output: 330


Skip superseded automatic applies before TF Apply.

queue: max keeps up to 100 push and schedule runs pending with manual recovery runs. actions/checkout uses each triggering revision, and concurrency order is not guaranteed. An older commit can therefore apply after a newer commit and roll infrastructure back. Skip superseded automatic runs, or isolate manual recovery while retaining one shared state lock.

🧰 Tools
🪛 actionlint (1.7.12)

[error] 45-45: unexpected key "queue" for "concurrency" section. expected one of "cancel-in-progress", "group"

(syntax-check)

🤖 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 @.github/workflows/apply.yaml around lines 28 - 45, Update the apply workflow
before the TF Apply step to skip queued automatic push or schedule runs that
have been superseded by a newer revision, while allowing manual recovery runs to
execute. Preserve serialization through the shared concurrency group for all
runs, and ensure the supersession check does not discard workflow_dispatch
recovery operations.

Comment thread .github/workflows/apply.yaml
eliorerz added a commit to eliorerz/github-config that referenced this pull request Aug 5, 2026
A single broken/unrefreshable resource aborts the entire tofu apply
atomically, blocking every other repo's pending Terraform changes with
no signal beyond a red Actions run. This has happened twice: the
host-management-openstack archived-repo bug (osac-project#165) and the orphaned
github_membership entries for users who left the org (osac-project#162). The
CaaS-Netris required-check rename sat un-applied for hours because of
the former, discovered only by chance while investigating an unrelated
CI queue backlog.

Two changes:

- File (or comment on) a tracking issue when tofu apply fails, so a
  stuck apply is never silent again. Uses the same GitHub App token
  already generated for tofu itself (issues:write is already granted),
  no new secrets needed.

- Add a one-time, manual `exclude_addresses` workflow_dispatch input
  (mirrors osac-project#162's `state_rm_addresses` pattern) that threads into
  `tofu apply -exclude=...`, letting a maintainer immediately unblock
  every other repo's changes when one resource is known-broken, while
  a permanent fix is prepared -- without giving up single-pass,
  dependency-complete apply for the normal case.

A permanent per-module `-target` loop was considered and rejected:
several modules share cross-module resources (e.g.
github_team.all["wg-infra"], referenced by ruleset_bypass_team_ids in
5+ repo modules), so looping per module would redundantly re-plan/
re-apply those shared resources on every iteration referencing them,
and gives up Terraform's whole-graph dependency ordering for no real
isolation benefit in the common case where nothing is broken.

Signed-off-by: Elior Erez <eerez@redhat.com>
Terraform's apply fails atomically on any resource whose live state
can no longer be refreshed (e.g. github_membership/github_team_membership
for a user who has left the org), blocking every other pending change
in the same apply -- including the archived=true settings from osac-project#161.
Mirrors the existing one-time TF import mechanism.
- Simplify the concurrency group to just github.workflow. This workflow
  has no pull_request trigger, so github.event.pull_request.number was
  always null and the group key collapsed to github.ref -- meaning a
  workflow_dispatch run against a non-default ref could run concurrently
  with a scheduled/push apply against main, racing on the same remote
  state. A single, unqualified group serializes every run of this
  workflow against every other, regardless of ref or trigger.
- Pass all parsed addresses to a single `tofu state rm` invocation
  instead of looping one call per address, so the state write happens
  once instead of N times.
- Pass state_rm_addresses through env + a quoted shell variable instead
  of interpolating the workflow expression directly into the script.

Signed-off-by: Elior Erez <eerez@redhat.com>
- queue: max on the concurrency group -- default queue behavior only
  keeps the single most-recently-queued run pending and cancels older
  ones, so a manual one-time recovery run (state rm, or the
  exclude_addresses escape hatch added in a follow-up PR) dispatched
  while a scheduled/push run is in progress could get silently dropped
  and replaced before it ever executes.
- Reject state_rm_addresses containing a newline before parsing --
  `read` without -a per-field handling would otherwise silently
  truncate at the first newline and drop later addresses. (The
  already-present `read -ra` already implies -r; CodeRabbit's specific
  claim that -r was missing didn't hold up against the actual
  committed content, verified via `git show`.)
- Drop the unused GITHUB_TOKEN from the TF State Remove step --
  `tofu state rm` is a pure state-file operation against the S3/AWS
  backend and never calls the GitHub provider API.

Signed-off-by: Elior Erez <eerez@redhat.com>
A stray comma or a typo'd leading "-" would otherwise be passed straight
to `tofu state rm`, which parses it as a CLI option rather than a
resource address (e.g. "-dry-run,github_membership.example"). Validate
each token before running the command.

Signed-off-by: Elior Erez <eerez@redhat.com>
@eliorerz
eliorerz force-pushed the osac-1737-fix-apply-orphaned-members branch from e5acfd6 to ca3c702 Compare August 5, 2026 23:09
@eliorerz
eliorerz merged commit 0aa4d0c into osac-project:main Aug 5, 2026
2 checks passed
@eliorerz
eliorerz deleted the osac-1737-fix-apply-orphaned-members branch August 5, 2026 23:10
eliorerz added a commit to eliorerz/github-config that referenced this pull request Aug 5, 2026
A single broken/unrefreshable resource aborts the entire tofu apply
atomically, blocking every other repo's pending Terraform changes with
no signal beyond a red Actions run. This has happened twice: the
host-management-openstack archived-repo bug (osac-project#165) and the orphaned
github_membership entries for users who left the org (osac-project#162). The
CaaS-Netris required-check rename sat un-applied for hours because of
the former, discovered only by chance while investigating an unrelated
CI queue backlog.

Two changes:

- File (or comment on) a tracking issue when tofu apply fails, so a
  stuck apply is never silent again. Uses the same GitHub App token
  already generated for tofu itself (issues:write is already granted),
  no new secrets needed.

- Add a one-time, manual `exclude_addresses` workflow_dispatch input
  (mirrors osac-project#162's `state_rm_addresses` pattern) that threads into
  `tofu apply -exclude=...`, letting a maintainer immediately unblock
  every other repo's changes when one resource is known-broken, while
  a permanent fix is prepared -- without giving up single-pass,
  dependency-complete apply for the normal case.

A permanent per-module `-target` loop was considered and rejected:
several modules share cross-module resources (e.g.
github_team.all["wg-infra"], referenced by ruleset_bypass_team_ids in
5+ repo modules), so looping per module would redundantly re-plan/
re-apply those shared resources on every iteration referencing them,
and gives up Terraform's whole-graph dependency ordering for no real
isolation benefit in the common case where nothing is broken.

Signed-off-by: Elior Erez <eerez@redhat.com>
eliorerz added a commit that referenced this pull request Aug 5, 2026
A single broken/unrefreshable resource aborts the entire tofu apply
atomically, blocking every other repo's pending Terraform changes with
no signal beyond a red Actions run. This has happened twice: the
host-management-openstack archived-repo bug (#165) and the orphaned
github_membership entries for users who left the org (#162). The
CaaS-Netris required-check rename sat un-applied for hours because of
the former, discovered only by chance while investigating an unrelated
CI queue backlog.

Two changes:

- File (or comment on) a tracking issue when tofu apply fails, so a
  stuck apply is never silent again. Uses the same GitHub App token
  already generated for tofu itself (issues:write is already granted),
  no new secrets needed.

- Add a one-time, manual `exclude_addresses` workflow_dispatch input
  (mirrors #162's `state_rm_addresses` pattern) that threads into
  `tofu apply -exclude=...`, letting a maintainer immediately unblock
  every other repo's changes when one resource is known-broken, while
  a permanent fix is prepared -- without giving up single-pass,
  dependency-complete apply for the normal case.

A permanent per-module `-target` loop was considered and rejected:
several modules share cross-module resources (e.g.
github_team.all["wg-infra"], referenced by ruleset_bypass_team_ids in
5+ repo modules), so looping per module would redundantly re-plan/
re-apply those shared resources on every iteration referencing them,
and gives up Terraform's whole-graph dependency ordering for no real
isolation benefit in the common case where nothing is broken.

Signed-off-by: Elior Erez <eerez@redhat.com>
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