Repository navigation
Update batch script tables - #31508
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis change renames and restructures two database tables related to batch script executions, updates associated SQL queries and code references, and adds a migration with a corresponding test. The schema now uses the names Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant DB
App->>DB: Insert into batch_activities (new batch execution)
App->>DB: Insert into batch_activity_host_results (host results)
App->>DB: Query batch_activity_host_results for errors
App->>DB: Query batch_activities for execution details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes This review involves checking a non-trivial schema migration, updates to multiple SQL queries and code references, and a migration test. The changes are broad but focused on renaming and restructuring, with minimal logic changes. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
server/datastore/mysql/hosts.go (1)
1190-1215: Missing parentheses change query meaningIn the “errored” branch the filter is built as:
... AND bsehr.error IS NOT NULL OR hsr.exit_code > 0Because
ANDbinds tighter thanOR, MySQL will read this as
(prev AND bsehr.error IS NOT NULL) OR hsr.exit_code > 0, returning every host whoseexit_code>0even for a different batch. Wrap the OR conditions:- batchScriptExecutionIDFilter += ` AND bsehr.error IS NOT NULL OR hsr.exit_code > 0` + batchScriptExecutionIDFilter += ` AND (bsehr.error IS NOT NULL OR hsr.exit_code > 0)`Also make sure
batch_activity_host_results.batch_execution_idandhost_script_results.execution_idare indexed, or the new LEFT JOINs will hurtListHostslatency on large fleets.
🧹 Nitpick comments (1)
server/datastore/mysql/schema.sql (1)
109-112: nullable columns without defaults may cause surprise
statusandactivity_typeallow NULL. In practice they will likely always be set ('started','script', etc.). Consider:status varchar(32) NOT NULL DEFAULT 'started', activity_type varchar(32) NOT NULL DEFAULT 'script',This guarantees data completeness and lets simple queries skip
COALESCE.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
server/datastore/mysql/hosts.go(2 hunks)server/datastore/mysql/migrations/tables/20250801083116_UpdateBatchScriptTables.go(1 hunks)server/datastore/mysql/migrations/tables/20250801083116_UpdateBatchScriptTables_test.go(1 hunks)server/datastore/mysql/schema.sql(2 hunks)server/datastore/mysql/scripts.go(4 hunks)server/datastore/mysql/scripts_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: in ee/server/service/hostidentity/depot/depot.go, the error from result.rowsaffected() is intentiona...
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:115-115
Timestamp: 2025-07-08T16:11:49.555Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the error from result.RowsAffected() is intentionally ignored because the information is only used for logging purposes, not for critical program logic.
Applied to files:
server/datastore/mysql/scripts_test.goserver/datastore/mysql/scripts.goserver/datastore/mysql/hosts.go
📚 Learning: in ee/server/service/hostidentity/depot/depot.go, the scep depot interface methods like put() do not...
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: ee/server/service/hostidentity/depot/depot.go:108-111
Timestamp: 2025-07-08T16:12:48.797Z
Learning: In ee/server/service/hostidentity/depot/depot.go, the SCEP depot interface methods like Put() do not accept context parameters, and the common_mysql.WithRetryTxx callback function type TxFn only receives a transaction parameter, not a context. Therefore, using context.Background() in tx.ExecContext calls within the transaction callback is the correct approach.
Applied to files:
server/datastore/mysql/scripts.go
📚 Learning: in the fleet codebase, down migration functions are intentionally left empty/no-op. the team does no...
Learnt from: getvictor
PR: fleetdm/fleet#30589
File: server/datastore/mysql/migrations/tables/20250707095725_HostIdentitySCEPCertificates.go:53-55
Timestamp: 2025-07-08T16:13:39.114Z
Learning: In the Fleet codebase, Down migration functions are intentionally left empty/no-op. The team does not implement rollback functionality for database migrations, so empty Down_* functions in migration files are correct and should not be flagged as issues.
Applied to files:
server/datastore/mysql/migrations/tables/20250801083116_UpdateBatchScriptTables.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (vuln, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (service, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (scripts, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (mysql, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (main, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:8.0.36, false)
- GitHub Check: test-go (integration-mdm, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-core, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (integration-enterprise, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: test-go (fleetctl, ubuntu-latest, mysql:9.3.0, false)
- GitHub Check: lint (macos-latest)
- GitHub Check: lint (windows-latest)
- GitHub Check: lint (ubuntu-latest)
- GitHub Check: build-binaries
- GitHub Check: publish
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (go)
🔇 Additional comments (16)
server/datastore/mysql/scripts_test.go (4)
844-844: Good documentation updateThe comment correctly reflects the new table name after the migration.
848-848: SQL query correctly updatedThe table name change from
batch_script_execution_host_resultstobatch_activity_host_resultsmatches the migration and maintains the same query logic.
853-853: Consistent documentation updateThe comment properly reflects the new table name, maintaining consistency with the previous update.
856-856: SQL query consistently updatedThis second query correctly uses the new table name, maintaining consistency with the earlier change on line 848.
server/datastore/mysql/migrations/tables/20250801083116_UpdateBatchScriptTables.go (5)
8-18: Migration setup looks goodThe migration registration and first table rename follow Fleet's standard patterns with proper error handling and descriptive error messages.
20-28: Column additions are well-designedThe new columns use appropriate data types:
int unsignedfor job_id andvarchar(255)for status/activity_type fields. Adding them as nullable initially is a safe migration approach.
30-44: Data updates are safe and logicalThe updates properly use
WHERE ... IS NULLclauses to avoid overwriting existing data, and the default values ('started' and 'script') are appropriate for existing batch script executions.
46-51: Second table rename is consistentThe rename from
batch_script_execution_host_resultstobatch_activity_host_resultsaligns with the broader refactoring and maintains consistent error handling.
56-58: Down migration correctly follows Fleet patternsThe empty Down migration function is intentional per Fleet's design - the team doesn't implement rollback functionality for database migrations, so this is the correct approach.
server/datastore/mysql/hosts.go (1)
547-551: Check cleanup list and foreign keysYou added
batch_activity_host_resultstohostRefs, so deleting a host now wipes its batch-activity rows. Nice. Please double-check the new table has ahost_idFK ON DELETE CASCADE (or at least an index) to keep this delete fast and safe.server/datastore/mysql/scripts.go (4)
868-868: Correct table name update for batch activities.The table rename from
batch_script_executionstobatch_activitiesaligns well with the migration strategy described in the PR objectives. This maintains the same functionality while using the new, more generic table name.
887-887: Proper table name update for host results.The rename from
batch_script_execution_host_resultstobatch_activity_host_resultsfollows the same pattern as the activities table. This keeps the host-specific execution tracking functionality intact while using the updated schema.
920-920: Correct query table reference update.The FROM clause properly references the new
batch_activity_host_resultstable name, maintaining the same query logic for gathering execution statistics.
934-934: Proper batch activities table reference.The query correctly uses the new
batch_activitiestable name while preserving the JOIN logic for retrieving script details in the summary.server/datastore/mysql/migrations/tables/20250801083116_UpdateBatchScriptTables_test.go (1)
10-39: Well-structured migration test with good coverage.This test does a nice job of validating the migration by:
- Setting up the pre-migration state with test data
- Applying the migration
- Verifying that new columns (
job_id,status,activity_type) have the expected default valuesThe use of
sql.NullStringandsql.NullInt64is appropriate for handling potentially null database values, and the assertions properly validate the migration behavior.One small suggestion: consider adding a comment explaining why
job_idis expected to be null whilestatusandactivity_typehave default values, as this helps document the migration's intended behavior.server/datastore/mysql/schema.sql (1)
1427-1434: auto-increment bump looks goodThe
AUTO_INCREMENTmoves to 407 and you insert row id 406. That keeps IDs monotonic, so the next migration lands on 407 automatically. Nice catch.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #31508 +/- ##
==========================================
+ Coverage 63.69% 63.71% +0.01%
==========================================
Files 1961 1962 +1
Lines 191141 191191 +50
Branches 6216 6216
==========================================
+ Hits 121755 121818 +63
+ Misses 59837 59823 -14
- Partials 9549 9550 +1
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:
|
|
Moving back to draft as we have more fields to add after discussion w/ @dantecatalfamo |
| return fmt.Errorf("failed to update activity_type in batch_activities: %w", err) | ||
| } | ||
|
|
||
| // Add an index on the new `status` column |
There was a problem hiding this comment.
As discussed with @dantecatalfamo, to help optimize the query for batch activity status.
for #31505
Checklist for submitter
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements)Testing
Added/updated automated tests
QA'd all new/changed functionality manually
no changed functionality
Database migrations
COLLATE utf8mb4_unicode_ci).collate for both these tables is previously set to
utf8mb4_0900_ai_ci, but they don't contain user-settable stringsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Chores