DO NOT MERGE: verify Sentry upload on CI - #5750
Conversation
Every PR build passes `-PskipSentryProguardMappingUpload=true`, so the upload tasks are never registered and #5708's new token wiring is never exercised. Without this, the first real test is the next code freeze. All three modules run because each declares its own Sentry project slug, newly inlined in #5708, and the token has to authenticate against all of them. --- Generated with the help of Claude Code, https://claude.ai/code Co-Authored-By: Claude Code Opus 5 <noreply@anthropic.com>
|
Claude finished @mokagio's task in 4m 9s —— View job Review: throwaway Sentry upload verification
The approach is sound and the script follows the house style closely — Three things, one of which I'd act on before letting the build decide anything. 1. 2. The task name is unverified, and a wrong one is indistinguishable from broken wiring. 3. The source-context upload isn't covered, which narrows what a green run proves. On the other axes: no security concern, the token is tested with Nothing here blocks the PR from doing its job. Worth folding in #1 before reading the result, since without it a red build may answer only a third of the question. |
Generated by 🚫 Danger |
| ./gradlew \ | ||
| :app:uploadSentryProguardMappingsRelease \ | ||
| :automotive:uploadSentryProguardMappingsRelease \ | ||
| :wear:uploadSentryProguardMappingsRelease |
There was a problem hiding this comment.
Without --continue, the first failing task aborts the build and the other two modules never run — which defeats the stated reason for running all three ("a typo in one would otherwise surface only at release"). If the app slug were wrong, this step tells you nothing about automotive or wear, and you'd need a second round trip through CI to find out.
--continue still fails the step, but it reports every module's outcome in one run:
| ./gradlew \ | |
| :app:uploadSentryProguardMappingsRelease \ | |
| :automotive:uploadSentryProguardMappingsRelease \ | |
| :wear:uploadSentryProguardMappingsRelease | |
| ./gradlew --continue \ | |
| :app:uploadSentryProguardMappingsRelease \ | |
| :automotive:uploadSentryProguardMappingsRelease \ | |
| :wear:uploadSentryProguardMappingsRelease |
Two related notes on this invocation:
The task name is unverified anywhere in the repo. grep over the tree and git log -S both turn up nothing but this script — no CI path has ever named a Sentry task, since they all skip the upload. If uploadSentryProguardMappingsRelease is not what the plugin (sentry-plugin = "6.18.0") registers, the step dies with Task 'uploadSentryProguardMappingsRelease' not found in project ':app', which reads like broken wiring rather than a typo in this script, and the PR's question goes unanswered. Worth confirming once with CI=true ./gradlew :app:tasks --all | grep -i sentry before trusting a red build here.
The source-context upload is not exercised. build.gradle.kts:487 sets includeSourceContext = shouldUploadDebugFiles from the same flag, and the real release path (build_bundle → gradle(task: ":#{app}:bundle", build_type: 'Release') in fastlane/Fastfile:596) passes no skipSentryProguardMappingUpload, so at code freeze the source-bundle upload runs against the same org and slugs for the first time too. Requesting only the mapping task leaves that half untested. Adding it would mean uploading PR-branch source code to the three production projects, which is a heavier side effect than the spare mapping the description accounts for — so it may well be the right call to leave out. If so, worth saying explicitly in the description, since "step passes ⇒ #5708 is safe to merge" is then narrower than it reads.
| echo "--- :key: Checking SENTRY_AUTH_TOKEN" | ||
|
|
||
| if [ -n "${SENTRY_AUTH_TOKEN:-}" ]; then | ||
| echo "SENTRY_AUTH_TOKEN is present" | ||
| else | ||
| echo "SENTRY_AUTH_TOKEN is absent" | ||
| fi |
There was a problem hiding this comment.
The token check itself is right: ${SENTRY_AUTH_TOKEN:-} keeps set -u happy and the value is never echoed, only its presence.
It's one variable short of covering the failure modes, though. build.gradle.kts:484-486 gates includeProguardMapping on System.getenv()["CI"].toBoolean(), and the plugin only registers the upload tasks when that is true — so if CI is unset or not literally true, the run fails with Task ... not found, indistinguishable from a wrong task name and nothing to do with the token. Buildkite exports CI=true, so this is about keeping the log self-diagnosing rather than a likely break, and it costs one line:
| echo "--- :key: Checking SENTRY_AUTH_TOKEN" | |
| if [ -n "${SENTRY_AUTH_TOKEN:-}" ]; then | |
| echo "SENTRY_AUTH_TOKEN is present" | |
| else | |
| echo "SENTRY_AUTH_TOKEN is absent" | |
| fi | |
| echo "--- :key: Checking Sentry upload preconditions" | |
| if [ -n "${SENTRY_AUTH_TOKEN:-}" ]; then | |
| echo "SENTRY_AUTH_TOKEN is present" | |
| else | |
| echo "SENTRY_AUTH_TOKEN is absent" | |
| fi | |
| # The upload tasks only exist when `CI` is truthy, because that is what gates | |
| # `includeProguardMapping` in `applyCommonSentryConfiguration`. An unexpected value here surfaces as | |
| # "task not found", which looks nothing like a token problem. | |
| echo "CI=${CI:-<unset>}" |
| # Throwaway, delete with `.buildkite/commands/verify-sentry-upload.sh`. | ||
| - label: ":sentry: Verify Sentry Upload" | ||
| command: ".buildkite/commands/verify-sentry-upload.sh" | ||
| plugins: [ $CI_TOOLKIT ] |
There was a problem hiding this comment.
Cosmetic, but it costs nothing to avoid: the step lands immediately above the ########## Optional Prototype Builds ########## banner (indented under the group that follows it), so the banner now reads as documentation for this step. Appending the step at the end of the file instead would both keep that banner attached to its group and make the eventual deletion a single trailing hunk.
The rest of the step is consistent with its neighbours — plugins: [ $CI_TOOLKIT ], and no agents: override so it inherits the top-level queue: "android", which is the queue the description wants to test the token on.
Omitting the should-skip-job.sh --job-type build guard that assemble-release-apk.sh and prototype-build.sh open with is also the right call here: .buildkite/** isn't in COMMON_PATTERNS, so it would never skip on this branch anyway, and you want this step to run unconditionally.
Do not merge. Stacked on #5708 purely to answer one question, then to be closed and deleted.
Description
#5708 moves the Sentry upload auth token out of the
configure-provisionedsecret.propertiesand into theSENTRY_AUTH_TOKENenvironment variable, and inlines the org and per-module project slugs. Nothing in normal CI exercises any of that: every build path passes-PskipSentryProguardMappingUpload=true, so the upload tasks are never registered. Without this, the first real test of the new wiring is the next code freeze.This adds a temporary step that does the upload for real, so we find out now.
uploadSentryProguardMappingsReleasepulls inminifyReleaseWithR8, which produces the mapping, but leaves packaging and signing out of the graph.app,automotiveandweareach declare their own Sentry project slug —pocket-casts-android,pocket-casts-automotive,pocket-casts-wear— newly inlined in Use public Sentry org/project, env-injected token #5708. The token has to authenticate against all three, so a typo in one would otherwise surface only at release.What the result means
androidqueue agents,providers.environmentVariablepicks it up, and thea8corg plus all three project slugs authenticate. Use public Sentry org/project, env-injected token #5708 is safe to merge.SENTRY_AUTH_TOKEN is not set (or is blank).) — the token is not exported on the queue and needs adding before Use public Sentry org/project, env-injected token #5708 merges. The log prints a bare present/absent line before the build starts, to tell this apart from a Gradle-side problem.sentry-cli— the token is there, but an org/project slug or the token's scope is wrong.Testing Instructions
a8c/pocket-casts-android,a8c/pocket-casts-automotiveanda8c/pocket-casts-wearin Sentry.Other failures on this branch are not expected. Nothing here should be "fixed" — close the PR once it has answered the question.
Checklist