Bootstrapping Android app - #36233
Conversation
GitHub Actions
Failure
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #36233 +/- ##
==========================================
+ Coverage 65.95% 65.96% +0.01%
==========================================
Files 2132 2133 +1
Lines 181575 181682 +107
Branches 7427 7451 +24
==========================================
+ Hits 119755 119850 +95
- Misses 50861 50866 +5
- Partials 10959 10966 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughIntegrates Android Management API (AMAPI) and WorkManager into the Fleet Android agent: adds periodic config checks, introduces an AMAPI notification service, updates manifest and docs for SHA256 fingerprint handling, adds dependencies/tests, and augments server-side Android policy payload with signing key certs and companion-app role. Changes
Sequence Diagram(s)sequenceDiagram
participant Android as Android System
participant App as Fleet Agent App
participant WM as WorkManager
participant Worker as ConfigCheckWorker
participant AMAPI as Android Management API
participant Notif as FleetNotificationReceiverService
Android->>App: App process start
App->>App: FleetApplication.onCreate()
App->>WM: enqueueUniquePeriodicWork("config_check_periodic", 15m)
App->>App: Log "Periodic config check scheduled"
Note over WM: Periodic trigger (~15m)
WM->>Worker: trigger doWork()
Worker->>Worker: Log "Periodic config check triggered"
Worker-->>WM: Result.success()
AMAPI->>Notif: Notification: App roles change
Notif->>Notif: onAppRolesSet(request)
Notif->>AMAPI: AppRolesSetResponse (default)
Notif->>Notif: Log "App roles set by Android Device Policy"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 full review |
✅ Actions performedFull review 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: 6
🧹 Nitpick comments (1)
android/app/src/test/java/com/fleetdm/agent/ConfigCheckWorkerTest.kt (1)
22-31: Test coverage is minimal but appropriate for the current stub implementation.The test only verifies that
doWork()returns success without checking any actual config-checking behavior. When the worker implementation is expanded to perform real configuration checks, add tests to verify:
- Actual config retrieval and processing
- Error handling scenarios
- Logging behavior
- Any side effects of the config check
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
android/README.md(1 hunks)android/app/build.gradle.kts(1 hunks)android/app/src/main/AndroidManifest.xml(2 hunks)android/app/src/main/java/com/fleetdm/agent/BootReceiver.kt(1 hunks)android/app/src/main/java/com/fleetdm/agent/ConfigCheckWorker.kt(1 hunks)android/app/src/main/java/com/fleetdm/agent/FleetApplication.kt(1 hunks)android/app/src/main/java/com/fleetdm/agent/FleetNotificationReceiverService.kt(1 hunks)android/app/src/main/java/com/fleetdm/agent/RestrictionsReceiver.kt(0 hunks)android/app/src/test/java/com/fleetdm/agent/ConfigCheckWorkerTest.kt(1 hunks)android/gradle/libs.versions.toml(2 hunks)go.mod(4 hunks)server/mdm/android/service/androidmgmt/policy_field_mask_test.go(1 hunks)server/mdm/android/service/service.go(1 hunks)
💤 Files with no reviewable changes (1)
- android/app/src/main/java/com/fleetdm/agent/RestrictionsReceiver.kt
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
⚙️ CodeRabbit configuration file
When reviewing SQL queries that are added or modified, ensure that appropriate filtering criteria are applied—especially when a query is intended to return data for a specific entity (e.g., a single host). Check for missing WHERE clauses or incorrect filtering that could lead to incorrect or non-deterministic results (e.g., returning the first row instead of the correct one). Flag any queries that may return unintended results due to lack of precise scoping.
Files:
server/mdm/android/service/androidmgmt/policy_field_mask_test.goserver/mdm/android/service/service.go
🧠 Learnings (1)
📚 Learning: 2025-08-08T08:32:31.529Z
Learnt from: getvictor
Repo: fleetdm/fleet PR: 31695
File: server/datastore/mysql/apple_mdm_test.go:132-132
Timestamp: 2025-08-08T08:32:31.529Z
Learning: Datastore.NewMDMWindowsConfigProfile signature is: NewMDMWindowsConfigProfile(ctx context.Context, cp fleet.MDMWindowsConfigProfile, usesFleetVars []string) (*fleet.MDMWindowsConfigProfile, error). Passing nil for usesFleetVars in tests denotes “no Fleet variables referenced” and is used consistently across the repo.
Applied to files:
server/mdm/android/service/service.go
🔇 Additional comments (9)
android/app/src/main/java/com/fleetdm/agent/BootReceiver.kt (1)
9-11: LGTM!Moving TAG to a companion object with
const valis idiomatic Kotlin and aligns with the pattern used in other files likeFleetApplication.kt.server/mdm/android/service/androidmgmt/policy_field_mask_test.go (1)
10-14: LGTM!The addition of
defaultApplicationSettingsto the expected mask aligns with the new policy fields. The assertion order change toexpectedMask, maskfollows the correct convention forassert.Equal(expected, actual).android/app/src/main/java/com/fleetdm/agent/FleetApplication.kt (2)
20-24: LGTM!The Application class correctly initializes logging and schedules the periodic config check during app startup.
26-42: Verify KEEP policy behavior for frequent process restarts.The
ExistingPeriodicWorkPolicy.KEEPpolicy ensures that if the app process is restarted multiple times (e.g., due to broadcasts), the existing periodic work will not be rescheduled. This is the correct choice for this use case.However, verify that this behavior is desired if you ever need to update the work request parameters (e.g., change the interval). With KEEP, you would need to explicitly cancel and re-enqueue the work to apply changes.
android/README.md (1)
112-119: LGTM!The deployment instructions are clear and include both required environment variables for the Fleet server.
android/app/build.gradle.kts (1)
110-114: LGTM!All new dependencies are correctly specified in
gradle/libs.versions.toml:
androidx-work-runtime-ktx(line 32, referencesworkManagerversion)amapi-sdk(line 34, referencesamapiv1.7.0)androidx-work-testing(line 33, referencesworkManagerversion)robolectric(line 35, referencesrobolectricv4.14)The dependencies appropriately support the WorkManager-based config checking and AMAPI integration introduced in this PR.
android/app/src/main/AndroidManifest.xml (2)
5-9: LGTM!The package visibility query for
com.google.android.apps.work.clouddpcis required for apps targeting Android 11 (API 30) and above to interact with the Android Device Policy app. This is correctly implemented according to the AMAPI SDK integration guide.
18-18: LGTM!The application class declaration references
FleetApplication, which aligns with the PR's objective to implement Android agent bootstrapping using WorkManager for periodic configuration checks.go.mod (1)
156-156: Dependency updates are compatible with the codebase.Verification confirms all updated Google Cloud dependencies (oauth2 v0.33.0, grpc v1.76.0, protobuf v1.36.10, and others) are compatible:
- No breaking changes apply: grpc v1.76.0 requires Go 1.24+, adds optional Balancer.ExitIdle method, and removes a rarely-used experimental env var—none of which affect this codebase (which uses Go 1.25.3, has no custom Balancer implementations, and doesn't use the removed env var).
- oauth2 and protobuf updates are patch-level with no API breaks.
- Android Management API integration (18+ files) uses standard googleapis patterns compatible with these versions.
- go.sum is consistent with go.mod.
| override fun doWork(): Result { | ||
| Log.i(TAG, "Periodic config check triggered") | ||
| return Result.success() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Stub implementation needs actual config checking logic.
The doWork() method currently only logs a message and returns success without performing any actual configuration checking. For a complete bootstrap implementation, consider adding:
- Retrieval of managed configurations via
RestrictionsManageror AMAPI - Processing of retrieved configuration data
- Error handling for network failures or invalid configurations
- Appropriate Result.retry() or Result.failure() returns for error cases
Is this stub intentional for incremental development, or should the implementation be completed in this PR?
Do you want me to generate a sample implementation that retrieves and processes managed configurations?
There was a problem hiding this comment.
Yes, this will be added later.
| * Custom Application class for Fleet Agent. | ||
| * Runs when the app process starts (triggered by broadcasts, not by user). | ||
| */ | ||
| class AgentApplication : Application() { |
There was a problem hiding this comment.
What is the significance of an Application as opposed to a Service?
There was a problem hiding this comment.
It is a singleton, and it is the first thing that runs. It is typically used for global application state and initialization.
sharon-fdm
left a comment
sharon-fdm
left a comment
There was a problem hiding this comment.
I only reviewed .github/workflows/test-android.yml
LGTM
3 participants
Related issue: Resolves #36202
Updated how Android agent starts. See README updates.
Checklist for submitter
Testing
Summary by CodeRabbit
New Features
Documentation
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.