feat(feature-flag): bundle catalog as Android raw resource and JVM classpath resource - #11364
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in the new test and build script (potential NPE and likely missing Gradle type import) plus an incomplete resource-name sanitization that can generate invalid Android resource filenames.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR packages the declarative feature-flag catalog for runtime consumption in both Android (as a generated res/raw resource) and JVM (as a classpath resource), enabling plain JVM unit tests to load the catalog without an Android runtime.
Changes:
- Add a cacheable Gradle task (
GenerateFeatureFlagRawResTask) that copies the catalog into a generated Androidres/rawdirectory with a sanitized resource name. - Wire the generated
res/directory into:core:featureflagAndroid resources and also bundle the catalog into the JVM target’s processed resources. - Add unit tests for the Gradle task and a JVM test verifying the catalog can be read from the classpath.
File summaries
| File | Description |
|---|---|
| core/featureflag/src/jvmTest/kotlin/net/thunderbird/core/featureflag/FeatureFlagCatalogResourceTest.kt | Adds a JVM test that reads the catalog from the classpath. |
| core/featureflag/build.gradle.kts | Wires catalog generation into Android resources and includes the catalog in JVM processed resources. |
| build-plugin/plugin/src/test/kotlin/net/thunderbird/gradle/plugin/featureflag/task/GenerateFeatureFlagRawResTaskTest.kt | Adds unit tests validating raw resource generation behavior. |
| build-plugin/plugin/src/main/kotlin/net/thunderbird/gradle/plugin/featureflag/task/GenerateFeatureFlagRawResTask.kt | Introduces the cacheable task that copies/sanitizes the catalog into res/raw. |
| build-plugin/plugin/build.gradle.kts | Adds test dependencies needed by the new plugin tests. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 4
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
…asspath resource - Add GenerateFeatureFlagRawResTask to copy catalog into res/raw for Android modules - Bundle catalog as classpath resource for JVM target to enable plain JVM unit tests - Add test coverage for resource generation and classpath loading
…esTask - Verify task is up-to-date when inputs unchanged - Verify task re-executes when catalog content changes - Verify task output is relocatable in build cache
728db09 to
9e56e65
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The new task currently lowercases resource names using the default locale (risking non-deterministic outputs/build-cache issues) and a couple of new tests can fail with an early NPE due to nullable classloader handling.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
Suppressed comments (3)
build-plugin/plugin/src/main/kotlin/net/thunderbird/gradle/plugin/featureflag/task/GenerateFeatureFlagRawResTask.kt:63
- Using
Locale.getDefault()makes the sanitized resource name depend on the machine/user locale (e.g. Turkish locale casing), which can break deterministic outputs and build cache relocatability. Use a locale-independent mapping (e.g.Locale.ROOT) for lowercasing resource names.
.lowercase(Locale.getDefault())
core/featureflag/src/jvmTest/kotlin/net/thunderbird/core/featureflag/FeatureFlagCatalogResourceTest.kt:18
FeatureFlagCatalogResourceTest::class.java.classLoaderis nullable, but the code dereferences it immediately; if it is ever null this will fail with a NullPointerException before the assertion. Make the failure explicit withrequireNotNull(...)so the test reports a clear cause.
val classLoader = FeatureFlagCatalogResourceTest::class.java.classLoader
build-plugin/plugin/src/test/kotlin/net/thunderbird/gradle/plugin/featureflag/task/GenerateFeatureFlagRawResTaskFunctionalTest.kt:30
javaClass.classLoaderis nullable in Kotlin; if it is null this line will throw a NullPointerException beforecheckNotNullruns. Wrap the classloader access inrequireNotNull(...)so a missing classloader fails with a clearer message.
val metadata = checkNotNull(javaClass.classLoader.getResourceAsStream(PLUGIN_METADATA_RESOURCE)) {
- Files reviewed: 6/6 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Contribution Summary
Linked Issue/Ticket: Resolves #11328
RFC / Technical Design (if applicable): RFC 0004: Add a Declarative Feature Flag Catalog / Technical Design 0002: Declarative Feature Flag Catalog
Description
AI Disclosure
Select one of the following (mandatory)
Contribution Checklist
gradlew spotlessCheckto check andgradlew spotlessApplyto format your source code; will be checked by CI).gradlew testDebugUnitTest; will be checked by CI).