[Build] Skip Remote Build Cache When Secrets Are Missing#16265
[Build] Skip Remote Build Cache When Secrets Are Missing#16265ParaskP7 wants to merge 2 commits into
Conversation
Fresh clones request the remote build cache by default (use_remote_build_cache_locally=true in developer.properties-example) but have no secrets applied, so settings evaluation failed with "The build requested remote build cache, but secrets file is not found" before any task could run — blocking external contributors entirely. Register the remote build cache only when secrets are applied. When they are missing, only a developer who explicitly opted in via their own developer.properties file still gets the hard stop, since they are expected to have secrets; when the flag is true only through the committed developer.properties-example fallback (a fresh clone), log a warning and continue with the local build cache instead. Developers with secrets applied keep the default-on remote build cache unchanged.
The unit tests, gradle cache, and merged manifest diff jobs only run Gradle tasks that work without secrets: build properties fall back to the committed defaults.properties, google-services.json falls back to its committed example file, and the remote build cache on CI authenticates via the GRADLE_CACHE_NODE_PASSWORD environment variable rather than the secrets file. Removing the gems and secrets setup from these jobs stops exposing secrets to jobs that do not need them and makes CI exercise the same no-secrets path that a fresh clone goes through, guarding it against future hard secrets dependencies. The other jobs keep configure_apply for a reason each: release-build obviously needs the upload keystore, the Play Store credentials, and sentry.properties for the source context upload; run-instrumented- tests needs .configure-files/firebase.secrets.json (the Firebase Test Lab service account Fladle authenticates with, which only exists after configure_apply) plus the real e2e API credentials baked into the instrumentation arguments; prototype-build only builds a JalapenoDebug APK that would compile without secrets, but the artifact is distributed to testers and must carry real OAuth and google-services values and be signed with the shared debug keystore to stay functional and updatable across CI agents.
ParaskP7
added
category: tooling
Generated by 🚫 Danger |
There was a problem hiding this comment.
Pull request overview
Adjusts build configuration so local developers (and CI) don’t fail hard when remote build cache is requested but secrets haven’t been applied, and removes redundant secrets installation from several Buildkite commands.
Changes:
- Gate local remote build cache configuration behind a
secretsApplied()check, with a warning-only fallback for fresh clones. - Refactor secrets checking by introducing a
secretsApplied()helper and reusing it inassertSecretsApplied(). - Remove
configure_apply(and gem setup) from select Buildkite commands that only run Gradle tasks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
settings.gradle |
Skips configuring local remote build cache when secrets are missing; refactors secrets checks into a helper. |
.buildkite/commands/run-unit-tests.sh |
Stops installing secrets/gems before running unit tests. |
.buildkite/commands/gradle-cache-build.sh |
Stops installing secrets/gems before assembling debug build for cache priming. |
.buildkite/commands/diff-merged-manifest.sh |
Stops installing secrets/gems before generating merged manifest diffs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## trunk #16265 +/- ##
=========================================
Coverage 41.15% 41.15%
- Complexity 13501 13502 +1
=========================================
Files 2503 2503
Lines 146465 146465
Branches 21761 21761
=========================================
+ Hits 60278 60279 +1
Misses 80089 80089
+ Partials 6098 6097 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review - Found 1 potential issue.
I also investigated the concern raised by Copilot on settings.gradle and confirmed it is valid — details in the inline comment below.
PR housekeeping
- Build-tooling / CI change (gradle build cache config + Buildkite scripts); no app-behaviour test plan required. The PR description clearly explains the what and why.
- Investigated Copilot's inline review comment on
settings.gradleand confirmed it with evidence fromdeveloper.properties-exampleand the property-merge logic.
Automatic review · claude-opus-4-8 · Workflow run
How to reply to a finding
Reply on this review (or inline at the line the finding refers to) with one of:
@claude addressed- I made the change. Bot verifies against the next diff before marking resolved.@claude rejected: <reason>- Will not fix; reason gets quoted on the next review.@claude not-applicable- Finding does not apply (wrong file, already covered elsewhere, etc.).
The bot honours these on the next review pass.
| if (secretsApplied()) { | ||
| checkForRemoteBuildCacheOptimizedExperience() | ||
|
|
||
| remote(HttpBuildCache) { | ||
| url = gradle.ext.secretProperties.get('gradleCacheNodeUrl') | ||
| push = false | ||
| credentials { | ||
| username = "developer" | ||
| password = gradle.ext.secretProperties.get('gradleCacheNodePassword') | ||
| } | ||
| } | ||
| } else if (file("${rootDir}/developer.properties").exists()) { | ||
| // The developer explicitly opted in to the remote build cache via their own | ||
| // developer.properties file, so they are expected to have secrets applied. | ||
| assertSecretsApplied() |
There was a problem hiding this comment.
I'm unsure about the code flow here. The way I read it is:
if secretsApplied {
...
} else if ... {
assertSecretsApplied // throws if secretsApplied == false
...
So, the else if branch runs only if secretsApplied == false but then it checks for the secrets to be applied, which we already know to be false, resulting in a throw.
What am I missing?
There was a problem hiding this comment.
👋 @mokagio and thanks for this comment, your reading is exactly right and the always-throws is intentional! The else if isn't there to re-check the secrets, it is there to distinguish who is missing them:
- A developer with their own
developer.properties(created by copyingdeveloper.properties-example, i.e. explicitly accepting theuse_remote_build_cache_locally=trueopt-in) but without secrets applied. That's almost certainly an internal developer who forgotconfigure_apply, and for them the branch deliberately keeps the pre-existing hard stop, with its actionable message. - A fresh clone (no personal
developer.properties, the flag istrueonly via thedeveloper.properties-examplefallback) never reaches that branch. It falls through to theelsebelow and gets the⚠️ warn-and-continue instead, which is the whole point of this PR.
So in that branch assertSecretsApplied() is effectively "fail with the canonical message": we know it will throw, and calling it (instead of inlining a throw) keeps the error message defined in one place and checkForRemoteBuildCacheOptimizedExperience() byte-identical to JP/WPAndroid's. There's more on the why (and the rejected alternatives) in the PR description's "Considered alternatives" section, plus the bot thread above where the "own developer.properties = explicit opt-in" contract is spelled out.
Let me know what you think, happy to revisit, but indeed, I thoughts about this, on various angled, before deciding to go with this approach. 💭
FYI: It is traveling day for me today, I might be slow to reply.
There was a problem hiding this comment.
I think @mokagio 's right on the flow and I'm not sure if the answer above addressed the concern: I don't think "asserting secrets applied after we check that secrets don't exist" is helpful here in any way.
Maybe instead, you just want a throw on line 98?
Closes: AINFRA-2560
Description
A fresh
git cloneof this repo cannot build the debug variant../gradlew assembleJalapenoDebugaborts during settings evaluation with:The cause is the combination of two deliberate behaviors:
developer.properties-exampleships withuse_remote_build_cache_locally=true(a deliberate default from #15189, which this PR keeps), andsettings.gradlefalls back to that example when the developer has nodeveloper.propertiesof their own — so every fresh clone implicitly "requests" the remote build cache and then hard-fails on the missing secrets it can never have. External contributors and secret-less machines are blocked before any task runs, even though the rest of the build degrades gracefully (secrets.properties→defaults.properties,google-services.json→ example file).This PR contains two commits:
developer.propertiesfile still gets the hard stop (they are expected to have secrets), while a fresh clone — where the flag istrueonly through the example fallback — gets a warning and continues with the local build cache. Developers with secrets applied keep the default-on remote build cache unchanged, andcheckForRemoteBuildCacheOptimizedExperience()stays aligned with the shared implementation in JP/WPAndroid. cd3de6dinstall_gems+configure_applyfrom the CI jobs that do not need secrets (unit tests,gradle cache,manifest diff), so CI stops exposing secrets to jobs that never consume them and permanently exercises the same no-secrets path a fresh clone goes through. The jobs that genuinely need secrets keep them:release builds(upload keystore, Play Store credentials,sentry.properties),instrumented tests(the Firebase Test Lab service account at.configure-files/firebase.secrets.jsonplus real e2e API credentials), andprototype builds(the distributed APK must carry real OAuth andgoogle-servicesvalues and be signed with the shared debug keystore). 88d0a0dThe resulting behavior matrix when
use_remote_build_cache_locally=true:developer.propertiesConsidered alternatives
use_remote_build_cache_locally=falseindeveloper.properties-examplefixes the fresh clone too, but reverts the deliberate default-on decision from #15189, so it was discarded.configure_applywould silently lose the remote cache behind an easily-ignored warning — the hard stop exists precisely for them.git config user.emailending in@automattic.com/@a8c.com: identity-based and covers new machines, but adds process execution to settings evaluation; kept as a fallback idea.The chosen design (hard stop only behind an explicit, personally-created
developer.properties) matches the semantics JP/WPAndroid already has: an explicit request for a secrets-dependent feature may fail hard; an implicit default never should.Test Steps
All on a machine without applied secrets (
~/.configure/woocommerce-android/secrets/secrets.propertiesabsent):developer.properties):./gradlew assembleJalapenoDebugsucceeds and logs thedeveloper.propertieswithuse_remote_build_cache_locally=true:./gradlew helpfails with the originalconfigure_applyerror.false:./gradlew helpsucceeds with the plain ℹ️ disabled message.unit tests,gradle cache, andmanifest diffjobs run withoutconfigure_applyand pass.Dependency CacheBuild: 40672Images/gif
N/A — build and CI configuration only.
RELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.