[AndroidManifest] Reveal Manifest Changes in PR #13582
Conversation
ParaskP7
added
status: do not merge
ParaskP7
force-pushed
the
manifest/reveal-changes-in-pr
branch
19 times, most recently
from
4a8b9a6 to
a2993ee
Compare
ParaskP7
force-pushed
the
manifest/reveal-changes-in-pr
branch
from
f962e5d to
3bbd5e2
Compare
ParaskP7
force-pushed
the
manifest/reveal-changes-in-pr
branch
from
3bbd5e2 to
b8d0a8c
Compare
ParaskP7
force-pushed
the
manifest/reveal-changes-in-pr
branch
from
b8d0a8c to
545e874
Compare
📝 WalkthroughWalkthroughThis pull request introduces several new build scripts for managing merged manifests. The new scripts (for diffing, restoring, and saving manifests) use a build variant argument and invoke associated functions and Fastlane commands. The Buildkite pipeline is updated to replace multiple linting and testing steps with a manifest diff step, and a new schedule for storing merged manifests is added. Additionally, the CI toolkit version is updated. The pull request also adds the Singular library as a dependency to both WooCommerce projects, along with corresponding version and repository configurations in Gradle. Changes
Sequence Diagram(s)sequenceDiagram
participant BK as Buildkite Pipeline
participant Diff as diff-merged-manifest.sh
participant Restore as restore-merged-manifest.sh
participant Cache as restore-cache.sh
participant Gem as install_gems
participant Fastlane as Fastlane
participant Gradle as Gradle
BK->>Diff: Trigger with BUILD_VARIANT
Diff->>Restore: Run for "WooCommerce" module with BUILD_VARIANT
Diff->>Restore: Run for "WooCommerce-Wear" module with BUILD_VARIANT
Diff->>Cache: Execute restore-cache.sh
Diff->>Gem: Install Ruby gems
Diff->>Fastlane: Run Fastlane command to apply configuration secrets
Diff->>Gradle: Execute process"${BUILD_VARIANT^}"Manifest
Gradle-->>Diff: Return merged manifest
Diff->>BK: Output merged manifest differences
sequenceDiagram
participant Scheduler as Buildkite Schedule
participant Save as save-merged-manifest.sh
participant Cache as restore-cache.sh
participant Gem as install_gems
participant Fastlane as Fastlane
participant Gradle as Gradle
participant SaveMF as save_android_merged_manifest
Scheduler->>Save: Trigger with BUILD_VARIANT
Save->>Cache: Execute restore-cache.sh
Save->>Gem: Install Ruby gems
Save->>Fastlane: Run Fastlane command for configuration secrets
Save->>Gradle: Run Gradle assemble command for merged manifest
Gradle-->>Save: Merged manifest created
Save->>SaveMF: Save merged manifest for "WooCommerce"
Save->>SaveMF: Save merged manifest for "WooCommerce-Wear"
Save->>Scheduler: Complete manifest storage process
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Project manifest changesThe following changes in --- ./build/reports/diff_manifest/WooCommerce/jalapenoDebug/trunk_AndroidManifest.txt 2025-02-26 16:04:50.181552385 +0000
+++ ./build/reports/diff_manifest/WooCommerce/jalapenoDebug/pr_AndroidManifest.txt 2025-02-26 16:04:50.191552366 +0000
@@ -56,6 +56,12 @@
<intent>
<action android:name="android.intent.action.GET_CONTENT" />
</intent>
+ <intent>
+ <action android:name="com.singular.preinstall.READ_PERMISSION_SINGULAR" />
+ </intent>
+
+ <package android:name="com.facebook.katana" />
+ <package android:name="com.instagram.android" />
</queries>
<uses-permission android:name="android.permission.BLUETOOTH_SCAN" />
@@ -80,6 +86,7 @@
<uses-permission android:name="android.permission.ACCESS_WIFI_STATE" /> <!-- Required by older versions of Google Play services to create IID tokens -->
<uses-permission android:name="com.google.android.c2dm.permission.RECEIVE" />
<uses-permission android:name="com.google.android.finsky.permission.BIND_GET_INSTALL_REFERRER_SERVICE" />
+ <uses-permission android:name="com.android.vending.CHECK_LICENSE" />
<uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED" />
<permission |
Project manifest changesThe following changes in --- ./build/reports/diff_manifest/WooCommerce-Wear/jalapenoDebug/trunk_AndroidManifest.txt 2025-02-26 16:04:51.241550361 +0000
+++ ./build/reports/diff_manifest/WooCommerce-Wear/jalapenoDebug/pr_AndroidManifest.txt 2025-02-26 16:04:51.241550361 +0000
@@ -20,10 +20,18 @@
<intent>
<action android:name="androidx.wear.tiles.action.BIND_UPDATE_REQUESTER" />
</intent>
+ <intent>
+ <action android:name="com.singular.preinstall.READ_PERMISSION_SINGULAR" />
+ </intent>
+
+ <package android:name="com.facebook.katana" />
+ <package android:name="com.instagram.android" />
</queries>
<uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED" />
<uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
+ <uses-permission android:name="com.google.android.finsky.permission.BIND_GET_INSTALL_REFERRER_SERVICE" />
+ <uses-permission android:name="com.android.vending.CHECK_LICENSE" />
<permission
android:name="com.woocommerce.android.prealpha.DYNAMIC_RECEIVER_NOT_EXPORTED_PERMISSION" |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
.buildkite/schedules/merged-manifest-storage.yml (1)
7-10: Manifest Storage Step Configuration
The step (lines 7–10) defines the command to invoke.buildkite/commands/save-merged-manifest.shwith the argumentjalapenoDebugand attaches the$CI_TOOLKITplugin. Please verify that the referenced script exists in the proper directory and that the argument aligns with your build configuration..buildkite/commands/restore-merged-manifest.sh (1)
1-7: Clarify Error Handling in the Restore Script
The script correctly uses#!/bin/bash -efor immediate exit on errors; however, the use of|| trueafter the call torestore_android_merged_manifestmay be non-obvious to new readers. Consider adding a brief inline comment to clarify that any errors from the restore command are intentionally ignored so that the script does not fail the build..buildkite/commands/save-merged-manifest.sh (1)
5-5: Execution of restore-cache.sh
The line"$(dirname "${BASH_SOURCE[0]}")/restore-cache.sh"executes the
restore-cache.shscript. For clarity and maintainability, adding a short comment to explain what cache is being restored and why would be beneficial..buildkite/commands/diff-merged-manifest.sh (1)
5-7: Consistent Invocation of Restore Functions
The script invokesrestore-merged-manifest.shfor both "WooCommerce" and "WooCommerce-Wear" modules. It would be helpful to add brief comments indicating the purpose of these calls—such as “Restoring merged manifest for module X”—to make the script easier to follow.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.buildkite/commands/diff-merged-manifest.sh(1 hunks).buildkite/commands/restore-merged-manifest.sh(1 hunks).buildkite/commands/save-merged-manifest.sh(1 hunks).buildkite/pipeline.yml(1 hunks).buildkite/schedules/merged-manifest-storage.yml(1 hunks).buildkite/shared-pipeline-vars(1 hunks)WooCommerce-Wear/build.gradle(1 hunks)WooCommerce/build.gradle(1 hunks)gradle/libs.versions.toml(2 hunks)settings.gradle(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .buildkite/shared-pipeline-vars
🔇 Additional comments (10)
.buildkite/schedules/merged-manifest-storage.yml (2)
1-3: New Buildkite Schedule File Introduced
This YAML file sets up a new Buildkite schedule for storing merged manifests. The header (lines 1–3) correctly specifies the schema for YAML validation.
4-6: Agents Block Configuration
The agents block (lines 4–6) configures the pipeline to use the "android" queue. This clearly directs the job to the correct agent pool.WooCommerce-Wear/build.gradle (1)
209-211: Singular Dependency Addition
A new dependency (implementation(libs.singular)) has been appended at line 210. Ensure that the version declared in yourgradle/libs.versions.toml(set to "12.6.1") and the corresponding Maven repository (defined insettings.gradle) are in sync. This change looks consistent with the modifications in the WooCommerce module.WooCommerce/build.gradle (1)
440-442: Singular Dependency Addition in WooCommerce Module
The addition ofimplementation(libs.singular)in the dependencies block (around line 441) integrates the Singular SDK into this module. Confirm that this dependency meets the module’s requirements and that any related configuration (e.g., ProGuard rules if needed) is appropriately updated.settings.gradle (1)
70-73: New Maven Repository for Singular SDK
Lines 70–73 add a new Maven repository with the URLhttps://maven.singular.net. This repository is essential for resolving the Singular SDK dependency. Please confirm that this URL is correct and that access permissions (if any) are properly configured.gradle/libs.versions.toml (2)
91-93: Singular Version Declaration Added
A new version entry for the Singular SDK (singular = "12.6.1") has been added. Ensure that this version is tested and compatible with your project’s requirements.
254-256: Singular Library Entry Defined
A matching library declaration (singular = { module = "com.singular.sdk:singular_sdk", version.ref = "singular" }) has been added in the libraries section. This entry helps manage the dependency centrally via the version defined above..buildkite/commands/save-merged-manifest.sh (1)
1-24: Overall Script Flow and Error Handling
This script follows a clear sequence—from setting the build variant, restoring the cache, and configuring tools, to assembling and saving merged manifests for both modules. The use ofset -euo pipefail(starting at line 7) enhances robustness. Double-check that the Gradle command with the uppercase conversion ("${BUILD_VARIANT^}") behaves as expected on your target environments..buildkite/pipeline.yml (1)
8-12: CI Pipeline Step Configuration
The new "💾 Restore and Diff Merged Manifest" step is well defined. The command references the correct script and passes the expectedjalapenoDebugargument. The use of artifact paths for diff reports looks consistent. Just verify that all referenced scripts (e.g.diff-merged-manifest.sh) are present and executable within the repository..buildkite/commands/diff-merged-manifest.sh (1)
13-28: Manifest Diff Process and External Function Reliance
The remaining steps—setting up Ruby gems, installing secrets, creating the merged manifest, and diffing the manifests for both modules—are clearly structured. Please ensure that helper functions likeinstall_gemsandcomment_with_manifest_diffare defined and available in the environment. Also, verify that the Bash string expansion ("${BUILD_VARIANT^}") for capitalizing the first letter works consistently in your target shell environments.
| #!/bin/bash | ||
|
|
||
| BUILD_VARIANT=$1 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Position Strict Error Handling Early
The script currently sets the strict error handling (set -euo pipefail) on line 11. To ensure that all commands (including the initial restore commands) are executed with strict error handling, consider moving this setting immediately after the shebang, at the top of the script.
| "$(dirname "${BASH_SOURCE[0]}")/restore-cache.sh" | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ordering and Error Handling of Cache Restoration
The call to restore-cache.sh (line 9) is executed before setting strict error handling on line 11. Relocating the set -euo pipefail to the very top (as suggested above) will ensure that any error during cache restoration is caught.
Project Thread: paaHJt-7Tl-p2
Depends On: a8c-ci-toolkit#152
Description
TODOTesting information
TODORELEASE-NOTES.txtif necessary. Use the "[Internal]" label for non-user-facing changes.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: