fix(arc): read the App's id from an existing App Secret - #17
Merged
Merged
Conversation
Mearman
force-pushed
the
fix/arc-app-id-from-secret
branch
from
September 26, 2026 05:11
4906f3c to
de92e4d
Compare
Mearman
changed the base branch from
main
to
fix/arc-fleet-includes-play-orgs
September 26, 2026 05:11
Mearman
force-pushed
the
fix/arc-app-id-from-secret
branch
2 times, most recently
from
September 26, 2026 05:27
0b2c011 to
3763b59
Compare
Mearman
marked this pull request as ready for review
September 26, 2026 05:35
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The role required every org's app_id even with github_runner_arc_manage_secrets false, when it writes no App Secret and reads the id from the existing Secret wherever it needs one, so a consumer had to read that Secret itself just to pass the id back. app_id is now required only on a host that writes the App Secret; the fleet-wide expansion no longer asks for it, since only each host's own check knows whether it writes the Secret. With an existing App Secret the role now checks it holds github_app_id, github_app_installation_id and github_app_private_key, and that an app_id the org still sets matches the Secret's github_app_id, before anything changes. The Secret lookup is no_log, and the checks loop over a reduction to names, key names and the App's id, so no failure or verbose output can print a Secret's data.
Mearman
force-pushed
the
fix/arc-app-id-from-secret
branch
from
September 26, 2026 05:36
3763b59 to
8d6ad74
Compare
|
🎉 This PR is included in version 1.2.2 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
This came out of a consumer adopting v1.2.0. It keeps its App Secret outside the role (
github_runner_arc_manage_secrets: false) and had to read that Secret itself, only to passgithub_app_idback in as the org'sapp_id.This is stacked on the fix that assembles the fleet from this host's orgs however they were supplied. Both change the same lines of
validate.yml, so this PR's base is that branch. Retarget this PR tomainbefore merging the other one with branch deletion, or merge the two as a stack.With
manage_secretsfalse the role never usesapp_id. It writes no App Secret, resolves no installation id, and the App-sourced pull token already reads the id from the Secret. Validation still required it for every org.app_idis now required only on a host that writes the App Secret, and the error message says so and names the other source. The fleet-wide expansion used for the cross-host checks no longer asks forapp_id, because only each host's own check knows whether that host writes the Secret.Since the Secret is now the only source, the existing-Secret check does a bit more, before anything changes. Each App Secret must hold
github_app_id,github_app_installation_idandgithub_app_private_key, and anapp_idthe org still sets must match the Secret'sgithub_app_id. The messages name the Secret, the missing keys or both ids. The lookup isno_log. The checks now loop over a reduction to names, key names and the App's id, not over the raw lookup results: a failing loop prints its items, and I caught a first version of this change printing a Secret's data that way. The checks live incheck_existing_secret_contents.yml, so they can be tested without a cluster.Testing:
app_idboth ways.tests/unit/test_existing_secrets.pydrives the checks at-vvvwith lookup results shaped ask8s_inforeturns them. It covers a complete Secret with and without a matchingapp_id, a mismatch, a missing key, a missing Secret and a host that writes its own Secrets, and asserts the key material never appears in the output.manage_secrets: falseand noapp_id, that fleet passes, where v1.2.0 fails with "app_id is required".app_idit fails with the mismatch message, and a scan of the run's log found no key material.