Conversation
Previously the label sync was a manual, interactive `docker run` (last released v0.0.2, 2021) with no scheduled trigger, so target repos drifted out of sync with the leader (giantswarm/giantswarm) even when correctly registered in data/customers.yaml. - cli.py: add a --yes flag so the sync can run unattended (it otherwise blocks on an interactive Y/N prompt), and read the token from the GITHUB_TOKEN env var so CI need not write the secret to disk. - Add a weekly scheduled GitHub Actions workflow (label-sync.yaml) that authenticates via a GitHub App installation token and runs the sync. workflow_dispatch supports a dry_run input for manual preview. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks for automating this — the workflow is well-scoped, action SHAs are pinned, and the inline comments explaining the App permission model and the zz_generated naming caveat are much appreciated. A couple of non-blocking points:
1. GITHUB_TOKEN silently overrides an explicit --token-path
In read_token, the env var wins unconditionally:
env_token = os.environ.get(TOKEN_ENV_VAR)
if env_token:
return env_token.strip()
with open(os.path.expanduser(path), "r") as input:
...This is exactly right for the CI use case. The edge case is local use: GITHUB_TOKEN is commonly exported on developer machines (gh CLI, etc.), so someone who explicitly passes --token-path /some/file would silently get the env token instead. Preferring the file when --token-path was passed explicitly (or differs from its default) would remove the footgun. Minor, and the README does document the precedence — flagging so it's a deliberate choice.
Side note: this diff also quietly fixes a real pre-existing bug — the old code did open(path) without os.path.expanduser, so the default ~/.github-token never actually resolved. Worth calling out in the description as a bonus fix.
2. (Optional nit) No failure signal for the scheduled run
A silently failing weekly cron is the classic way sync drift creeps back in — the exact problem this PR solves. A Slack/notification step on failure would make a broken Monday run visible. Purely optional.
Generated by Claude Code
…eers on failed cron Address review feedback on #116: - read_token: an explicit --token-path now takes precedence over the GITHUB_TOKEN env var, so a token exported for other tools (e.g. gh CLI) no longer silently overrides a file the user asked for. Precedence is now: explicit --token-path > GITHUB_TOKEN > ~/.github-token. - label-sync.yaml: post a warning with the run log link to #team-planeteers when a scheduled run fails, following the pattern used by the team-*-daily-digest workflows in giantswarm/github. - README: document both.
|
Thanks for the review! Addressed in 1d28a73: 1. Token precedence — an explicit 2. Failure signal — added a |
|
Both improvements are applied as suggested by @puja108: explicit |
puja108
left a comment
There was a problem hiding this comment.
Re-reviewed at 1d28a73. Both points from the previous round are addressed:
1. Token precedence — fixed as intended. --token-path defaults to None, and read_token only consults GITHUB_TOKEN when no path was passed. The README's documented docker run --token-path /home/user/.github-token therefore keeps working regardless of what's exported in the shell. Help text and README match the behaviour.
2. Failure signal — the Report failed scheduled run to Slack step is there, correctly gated on failure() && github.event_name == 'schedule'. One gap in it, see below. I can't verify the TEAM_PLANETEERS_SLACK_WEBHOOK_URL secret or the webhook test from here — taking your word on that.
Three follow-ups, none of them regressions from this diff, but the last two only start to matter because this PR makes the run unattended.
a) The Slack step can fail silently (non-blocking, one-word fix)
curl -sS -o /dev/null -w 'HTTP %{http_code}\n' -X POST ... "$SLACK_WEBHOOK_URL"
curl without --fail exits 0 on 403/404/invalid_token, so a rotated or mistyped webhook makes the alerting step pass green while the alert goes nowhere — the same silent-failure mode the step exists to prevent. set -euo pipefail doesn't help here. Adding --fail (or --fail-with-body) makes a dead webhook show up as a red step.
b) The execute phase hardcodes the org for customer repos
Read phase uses the per-entry org:
target_labels[cr['repository']], _ = read_repo_labels(g, cr['organization'], cr['repository'], config['rules'])Execute phase does not — it rebuilds every handle under the config org (giantswarm):
for repo in target_labels.keys():
repo_handlers[repo] = repo = g.get_repo(f"{config['github']['organization']}/{repo}")target_labels is keyed by bare repository name, so for any customers.yaml entry whose organization is not giantswarm, the write targets giantswarm/<name> — a 404 (uncaught UnknownObjectException, so the weekly run dies mid-plan) or, if a same-named repo happens to exist in our org, label writes into the wrong repository. Same-named repos across two customer orgs would also collide on the dict key. Interactively someone would notice; on a Monday cron nobody is watching. If every entry in data/customers.yaml is in the giantswarm org today this is latent — I can't read that file to check — but it's worth confirming before the schedule goes live, and keying target_labels by org/name would remove the class of bug.
c) The per-label error handler raises instead of catching
except github.GithubException.GithubException as e:github.GithubException is the exception class, not the submodule, so this attribute lookup raises AttributeError the moment a real GithubException occurs (verified against the pinned PyGithub 2.10.0). Effect: one API error on create_label aborts the run with a traceback and a half-applied plan rather than logging and continuing. It does fail loudly, so the Slack alert fires — but except github.GithubException as e: is what was meant. Note also that edit() in the JOB_ACTION_EDIT branch has no handler at all, so error behaviour differs between create and update.
Nothing here blocks merging the automation itself; (b) is the one I'd check before the first scheduled run fires.
Generated by Claude Code
puja108
left a comment
There was a problem hiding this comment.
Re-review at 1d28a73, now with the findings anchored to the code. Both points from the previous round are properly addressed:
- Token precedence —
--token-pathdefaults toNoneandread_tokenonly consultsGITHUB_TOKENwhen no path was passed, so the README'sdocker run --token-path ...is unaffected by an exportedGITHUB_TOKEN. Help text and README match. - Failure signal — the Slack step is correctly gated on
failure() && github.event_name == 'schedule'. One gap in itscurlinvocation, inline.
Confirmed offline that all customers.yaml entries are in the giantswarm org, so the org-handling note on cli.py is latent rather than a live bug — downgraded to a follow-up. I can't verify the TEAM_PLANETEERS_SLACK_WEBHOOK_URL secret or your webhook test from here; taking your word on that.
Only the curl --fail nit is worth fixing in this PR; the two cli.py notes are pre-existing and can be follow-ups.
Generated by Claude Code
| set -euo pipefail | ||
| jq -n --arg text ":warning: Weekly label sync to customer repos failed. Run log: $RUN_URL" \ | ||
| '{text: $text}' \ | ||
| | curl -sS -o /dev/null -w 'HTTP %{http_code}\n' -X POST -H 'Content-type: application/json' \ | ||
| --data @- "$SLACK_WEBHOOK_URL" |
There was a problem hiding this comment.
curl without --fail exits 0 on 403/404/invalid_token, so a rotated or mistyped webhook makes this step pass green while the alert goes nowhere — the exact silent-failure mode the step exists to prevent. set -euo pipefail doesn't cover it, since curl itself succeeded. -w 'HTTP %{http_code}' puts the status in the log, but nobody reads the log of a green run.
| set -euo pipefail | |
| jq -n --arg text ":warning: Weekly label sync to customer repos failed. Run log: $RUN_URL" \ | |
| '{text: $text}' \ | |
| | curl -sS -o /dev/null -w 'HTTP %{http_code}\n' -X POST -H 'Content-type: application/json' \ | |
| --data @- "$SLACK_WEBHOOK_URL" | |
| set -euo pipefail | |
| jq -n --arg text ":warning: Weekly label sync to customer repos failed. Run log: $RUN_URL" \ | |
| '{text: $text}' \ | |
| | curl -sS --fail-with-body -X POST -H 'Content-type: application/json' \ | |
| --data @- "$SLACK_WEBHOOK_URL" |
Generated by Claude Code
There was a problem hiding this comment.
Applied as suggested in 91f767c: --fail-with-body replaces -o /dev/null -w 'HTTP %{http_code}', so a rejected webhook call now fails the step and Slack's response body lands in the log instead of a green step with HTTP 403.
There was a problem hiding this comment.
Line 112 — the per-label error handler raises instead of catching (pre-existing, outside this diff, so no inline anchor)
except github.GithubException.GithubException as e:github.GithubException is the exception class, not the submodule, so this attribute lookup raises AttributeError the moment a real GithubException occurs. Verified against the pinned PyGithub 2.10.0:
>>> github.GithubException
<class 'github.GithubException.GithubException'>
>>> github.GithubException.GithubException
AttributeError: type object 'GithubException' has no attribute 'GithubException'
Effect once this runs unattended: a single API error on create_label (rate limit, a repo the App isn't installed on, a permissions gap) aborts the run with a traceback and a half-applied plan, instead of logging and continuing to the next label. It does at least fail loudly, so the new Slack step fires. except github.GithubException as e: is what was meant.
Related, same block: the JOB_ACTION_EDIT branch calls target_labels[repo][label].edit(...) at line 118 with no handler at all, so create and update behave differently on error. Worth making them consistent while you're in there.
Generated by Claude Code
There was a problem hiding this comment.
Fixed in 36b3f20. The handler is except github.GithubException as e: and the edit() branch sits in the same try, so create and update behave the same on an API error: log ERROR: ... and continue with the next job.
One deliberate addition beyond the literal fix: the loop counts failures and, after the plan has run through, calls error() so the run exits 1 when anything failed. With the corrected handler alone a run with N failed labels would have exited 0 and the Slack step would never fire — the "fails loudly" property you noted would have been lost. Now the plan still runs to completion and the run still ends red.
There was a problem hiding this comment.
Line 103 — execute phase hardcodes the org for customer repos (pre-existing, outside this diff; latent, not a live bug — confirmed all customer repos are in the giantswarm org today)
Read phase honours the per-entry org, execute phase does not:
# read
target_labels[cr['repository']], _ = read_repo_labels(g, cr['organization'], cr['repository'], config['rules'])
# execute
repo_handlers[repo] = repo = g.get_repo(f"{config['github']['organization']}/{repo}")Since every customers.yaml entry is under giantswarm, both resolve to the same repo and nothing is wrong in practice — flagging only because the unattended weekly run changes the blast radius if that ever stops being true: the first non-giantswarm entry would either 404 mid-plan or, with a same-named repo in our org, write labels into the wrong repository, with no human watching. Keying target_labels by org/name and reusing cr['organization'] in the execute phase would close it off; a follow-up issue is fine, not this PR.
Generated by Claude Code
There was a problem hiding this comment.
Agreed, keeping this out of the PR. Tracked in #119 with your proposed fix: key target_labels by org/name and reuse cr['organization'] in the execute phase.
curl exits 0 on an HTTP 4xx/5xx unless told otherwise, so a rejected webhook call left the failure step green and the alert never arrived. --fail-with-body makes the step fail and prints Slack's response.
…d on failures `except github.GithubException.GithubException` raises AttributeError on PyGithub 2.x, because `github.GithubException` already is the exception class. Any API error on create_label therefore crashed the run instead of being logged. The update branch had no handler at all. Both branches now share one handler: log the error, continue with the rest of the plan, and exit 1 at the end when anything failed, so an unattended run does not look green while labels were not applied.
|
@puja108 round-2 follow-ups are in at 36b3f20:
Ready for another look. |
Previously the label sync was a manual, interactive
docker run(last released v0.0.2, 2021) with no scheduled trigger, so target repos drifted out of sync with the leader (giantswarm/giantswarm) even when correctly registered in data/customers.yaml.read_tokennow appliesos.path.expanduser, so the default--token-pathof~/.github-tokenactually resolves (previouslyopen()got the literal~, so the default never worked).Review follow-ups (see thread below):
Token precedence is now: explicit
--token-path>GITHUB_TOKENenv var >~/.github-token, so aGITHUB_TOKENexported for other tools does not silently override a file the user asked for.A failed scheduled run posts a warning with the run log link to
#team-planeteers(TEAM_PLANETEERS_SLACK_WEBHOOK_URLsecret), following theteam-*-daily-digestworkflows ingiantswarm/github.The Slack step fails on a rejected webhook call (
curl --fail-with-body). The execute phase catchesGithubExceptionon both create and update, logs it, finishes the plan, and exits 1 if any job failed so the schedule alert fires. Hardcoded org in the execute phase → Execute phase hardcodes the giantswarm org for customer repos #119.