Read build properties from local.properties before user-level gradle.properties#3568
Read build properties from local.properties before user-level gradle.properties#3568
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the Gradle build configuration in Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/build.gradle`:
- Around line 173-175: The loader closure loadProp currently prefers
localProperties when localProperties.containsKey(key) is true even if its value
is an empty string, which shadows project.properties; change the logic in
loadProp to read the value = localProperties.getProperty(key) and if that value
is null or empty/blank (use trim emptiness check) then fall back to
project.properties[key] ?: defaultValue, otherwise return the non-empty local
value—ensure you reference loadProp, localProperties.getProperty(key), and
project.properties[key] in the fix.
| def loadProp = { String key, def defaultValue = '' -> | ||
| localProperties.containsKey(key) ? localProperties.getProperty(key) : (project.properties[key] ?: defaultValue) | ||
| } |
There was a problem hiding this comment.
Empty value in local.properties silently shadows project.properties.
localProperties.containsKey(key) is true even when the stored value is an empty string (e.g., MAPBOX_SDK_API_KEY= in local.properties). In that case getProperty(key) returns "" and the project.properties fallback is never reached.
The old code project.properties['KEY'] ?: default relied on Groovy truth — an empty string was falsy and would fall through to the default. The new code intentionally inverts priority, but a developer who writes an empty entry in local.properties will silently receive "" even if a real value is set in ~/.gradle/gradle.properties.
Consider guarding against empty values to preserve the fallback chain:
🛠️ Proposed fix
def loadProp = { String key, def defaultValue = '' ->
- localProperties.containsKey(key) ? localProperties.getProperty(key) : (project.properties[key] ?: defaultValue)
+ def localVal = localProperties.getProperty(key)
+ (localVal != null && !localVal.isEmpty()) ? localVal : (project.properties[key] ?: defaultValue)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def loadProp = { String key, def defaultValue = '' -> | |
| localProperties.containsKey(key) ? localProperties.getProperty(key) : (project.properties[key] ?: defaultValue) | |
| } | |
| def loadProp = { String key, def defaultValue = '' -> | |
| def localVal = localProperties.getProperty(key) | |
| (localVal != null && !localVal.isEmpty()) ? localVal : (project.properties[key] ?: defaultValue) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/build.gradle` around lines 173 - 175, The loader closure loadProp
currently prefers localProperties when localProperties.containsKey(key) is true
even if its value is an empty string, which shadows project.properties; change
the logic in loadProp to read the value = localProperties.getProperty(key) and
if that value is null or empty/blank (use trim emptiness check) then fall back
to project.properties[key] ?: defaultValue, otherwise return the non-empty local
value—ensure you reference loadProp, localProperties.getProperty(key), and
project.properties[key] in the fix.
There was a problem hiding this comment.
@avazirna seems like this would be a nice-to-have tweak as well?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@conroy-ricketts let's think about it together. There is another perspective to this, If in the local.properties the value is an empty string, i.e. an empty String can be a valid value, and that's the value that should prevail. what do you think? I'm inclined to leave as-is
There was a problem hiding this comment.
@avazirna Ah yeah that's a good point. I think it makes sense to leave this as-is because if the value is an empty string then that is likely either intentional or a mistake by the developer
avazirna
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
| def loadProp = { String key, def defaultValue = '' -> | ||
| localProperties.containsKey(key) ? localProperties.getProperty(key) : (project.properties[key] ?: defaultValue) | ||
| } |
There was a problem hiding this comment.
@avazirna seems like this would be a nice-to-have tweak as well?
Replace double-quoted string literals that contain no GString interpolation with single-quoted strings. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
avazirna
dismissed stale reviews from conroy-ricketts and shubham1g5
via
389b585
3 participants
Product Description
This stemmed from a retro following an android secret was accidentally leaked in a PR. During the retro, it was noted that developers, particularly new team members, sometimes get confused about where secrets should be stored, so it was suggested to read from the 'local.properties' first. Since
local.propertiesis gitignored by default in Android projects, so it is safe for per-developer overrides without risk of committing secrets. Build server behaviour is unchanged since build servers do not have alocal.propertiesfile.Technical Summary
Previously, all build properties (API keys, signing config, test credentials) were read exclusively from
project.properties, which resolves user-level~/.gradle/gradle.propertieswith higher priority than the project-level file. This made it inconvenient to override properties locally without touching the sharedgradle.properties.Safety Assurance
Safety story
Built successfully locally.
Labels and Review