43962 vpp managed config migration - #44435
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44435 +/- ##
==========================================
+ Coverage 66.75% 66.77% +0.01%
==========================================
Files 2633 2634 +1
Lines 211736 211791 +55
Branches 9387 9387
==========================================
+ Hits 141354 141433 +79
+ Misses 57541 57524 -17
+ Partials 12841 12834 -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:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
WalkthroughAdds a MySQL migration that creates two configuration tables: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@server/datastore/mysql/migrations/tables/20260429180725_CreateTableAppConfigurations.go`:
- Around line 34-49: The uniqueness/indexing is wrong:
in_house_app_configurations has UNIQUE KEY idx_in_house_app_config_team_app
(team_id, in_house_app_id) which allows duplicate in_house_app_id across teams
and does not ensure team_id matches the parent in_house_apps row; fix by either
making the config keyed by the app row itself (replace the unique index with
UNIQUE (in_house_app_id)) or enforce referential integrity by adding a composite
foreign key that ties (in_house_app_id, team_id) to a corresponding UNIQUE or
PRIMARY KEY on in_house_apps (id, team_id) and keep
idx_in_house_app_config_team_app accordingly; update the migration DDL around
in_house_app_configurations, idx_in_house_app_config_team_app, and
fk_in_house_app_configurations to implement one of these options so stored
team_id cannot diverge from the parent in_house_apps row.
- Around line 13-29: The vpp_app_configurations table is currently only
constrained to vpp_apps via the fk_vpp_app_configurations_app foreign key
(application_id, platform), allowing any team_id to attach configs; update the
schema so the row is owned by the team-scoped relation instead: modify or add a
foreign key that references the team-scoped VPP relation (vpp_apps_teams) using
the composite key including team_id, application_id and platform (e.g. FOREIGN
KEY (team_id, application_id, platform) REFERENCES vpp_apps_teams (team_id,
application_id, platform) ON DELETE CASCADE), and keep or adjust the UNIQUE KEY
idx_vpp_app_config_team_app_platform as needed; ensure the referenced column
names match vpp_apps_teams and remove or replace the existing
fk_vpp_app_configurations_app constraint accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: fa32680d-5c24-454e-8716-7f19183d8c96
📒 Files selected for processing (3)
server/datastore/mysql/migrations/tables/20260429180725_CreateTableAppConfigurations.goserver/datastore/mysql/migrations/tables/20260429180725_CreateTableAppConfigurations_test.goserver/datastore/mysql/schema.sql
The parent in_house_apps row already pins one team_id and platform via its own columns, so storing team_id on the config table was denormalized and let team_id diverge from the parent. The unique key now keys on in_house_app_id alone (one config per app row), and the team_id column is gone entirely. Test asserts the duplicate in_house_app_id is rejected.
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 `@server/datastore/mysql/schema.sql`:
- Around line 3108-3120: The schema creates a single vpp_app_configurations
table with a platform discriminator and a foreign key to vpp_apps (adam_id,
platform), but the design requirement mandates separate scoped storage for iOS
and iPadOS rather than a shared table; split the single table into two explicit
tables (e.g., vpp_app_configurations_ios and vpp_app_configurations_ipados) or
otherwise scope rows by separate tables, remove the platform column and adjust
constraints and keys (replace UNIQUE KEY idx_vpp_app_config_team_app_platform
and FK fk_vpp_app_configurations_app) so each new table references the correct
vpp_apps entry (by adam_id and its fixed platform) and enforce ON DELETE CASCADE
per-table to match the objective and avoid migration churn.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 36373be9-5e7d-4533-a878-1204334ca547
📒 Files selected for processing (3)
server/datastore/mysql/migrations/tables/20260429180725_CreateTableAppConfigurations.goserver/datastore/mysql/migrations/tables/20260429180725_CreateTableAppConfigurations_test.goserver/datastore/mysql/schema.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- server/datastore/mysql/migrations/tables/20260429180725_CreateTableAppConfigurations.go
| CREATE TABLE `vpp_app_configurations` ( | ||
| `id` int unsigned NOT NULL AUTO_INCREMENT, | ||
| `application_id` varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| `team_id` int unsigned NOT NULL, | ||
| `platform` varchar(10) CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| `configuration` mediumtext COLLATE utf8mb4_unicode_ci NOT NULL, | ||
| `created_at` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6), | ||
| `updated_at` timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6), | ||
| PRIMARY KEY (`id`), | ||
| UNIQUE KEY `idx_vpp_app_config_team_app_platform` (`team_id`,`application_id`,`platform`), | ||
| KEY `fk_vpp_app_configurations_app` (`application_id`,`platform`), | ||
| CONSTRAINT `fk_vpp_app_configurations_app` FOREIGN KEY (`application_id`, `platform`) REFERENCES `vpp_apps` (`adam_id`, `platform`) ON DELETE CASCADE | ||
| ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; |
There was a problem hiding this comment.
Schema design conflicts with the stated iOS/iPadOS storage decision.
This introduces a single vpp_app_configurations table with a platform discriminator, but the linked objective explicitly calls for iOS and iPadOS to be scoped separately (not a shared table + platform). Merging this as-is risks immediate follow-up migration churn and downstream contract mismatch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/datastore/mysql/schema.sql` around lines 3108 - 3120, The schema
creates a single vpp_app_configurations table with a platform discriminator and
a foreign key to vpp_apps (adam_id, platform), but the design requirement
mandates separate scoped storage for iOS and iPadOS rather than a shared table;
split the single table into two explicit tables (e.g.,
vpp_app_configurations_ios and vpp_app_configurations_ipados) or otherwise scope
rows by separate tables, remove the platform column and adjust constraints and
keys (replace UNIQUE KEY idx_vpp_app_config_team_app_platform and FK
fk_vpp_app_configurations_app) so each new table references the correct vpp_apps
entry (by adam_id and its fixed platform) and enforce ON DELETE CASCADE
per-table to match the objective and avoid migration churn.
There was a problem hiding this comment.
We decided to go with a shared table https://fleetdm.slack.com/archives/C086V2QK76X/p1777478870183119?thread_ts=1777477120.314599&cid=C086V2QK76X
Migration #44435 dropped team_id from in_house_app_configurations (the parent in_house_apps row already pins team and platform). Updates the five in-house datastore methods, the four interface signatures, and the regenerated mocks to match.
2 participants
Related issue: Resolves #43962
Adds two tables:
vpp_app_configurationsandin_house_app_configurationsvpp_app_configurationshasteam_idunsigned not nullable, rather thanteam_idnullable +global_or_team_id. This is following the pattern insoftware_title_display_namesandsoftware_title_icons, since software installers are team only and cannot be global.android_app_configurationsuses team_id + global_or_team_id but that seems to be unnecessary.in_house_app_configurationskeys onin_house_app_idonly — the parentin_house_appsrow already pins the team and platform.Both use MEDIUMTEXT to store the XML configuration.
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Database migrations
COLLATE utf8mb4_unicode_ci).Summary by CodeRabbit