feat: add schedule validation and time trigger creation logic#82
Conversation
WalkthroughThe changes introduce protobuf-based scheduling types and enums, refactor scheduler logic to use these types, and enhance validation for policy schedules. Error handling is improved in the scheduler service initialization. The frequency field for time triggers is migrated from string to integer in both code and database schema. A new test validates time trigger operations. Minor formatting and dependency version updates are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant Server
participant SchedulerService
participant Logger
Server->>SchedulerService: NewSchedulerService(...)
alt Error
SchedulerService-->>Server: error
Server->>Logger: Log fatal error
Server-->>Caller: return nil
else Success
SchedulerService-->>Server: SchedulerService instance, nil
Server->>Caller: return Server instance
end
sequenceDiagram
participant Plugin
participant Schedule
participant Validator
Plugin->>Validator: validateSchedule(schedule)
alt schedule is nil or frequency unspecified or start time missing
Validator-->>Plugin: return error
else end time before start time
Validator-->>Plugin: return error
else
Validator-->>Plugin: success
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)level=warning msg="[runner] Can't run linter goanalysis_metalinter: buildir: failed to load package session: could not load export data: no export data for "github.com/vultisig/go-wrappers/go-dkls/sessions"" 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code Graph Analysis (1)storage/postgres/time_trigger.go (2)
🔇 Additional comments (3)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR introduces schedule validation and time trigger creation logic for the plugin. Key changes include adding tests for time triggers, updating the scheduling logic in the scheduler and policy components, and adjusting the database migration for time trigger frequency storage.
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| storage/postgres/time_trigger_test.go | Adds tests for creating, retrieving, and deleting time triggers. |
| storage/postgres/migrations/plugin/20241211182120_add_time_triggers.sql | Updates the frequency column type from TEXT to INT. |
| storage/postgres/db.go | Refactors receiver names and updates migration function usage. |
| plugin/payroll/policy.go | Introduces schedule validation and renames rule validation functions. |
| internal/types/time_trigger.go | Changes Frequency field type to use the enum rtypes.ScheduleFrequency. |
| internal/scheduler/scheduler.go | Refactors scheduler service and schedule creation logic to use updated types. |
| go.mod | Upgrades package dependency for recipes. |
| cmd/payroll/worker/main.go | Minor formatting change. |
| api/server.go | Adjusts scheduler service initialization to handle errors. |
Comments suppressed due to low confidence (1)
storage/postgres/time_trigger_test.go:16
- The test is being skipped unconditionally, which reduces test coverage for the time trigger functionality. If intentional, consider adding a comment explaining the rationale.
t.SkipNow()
![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
🔭 Outside diff range comments (1)
plugin/payroll/policy.go (1)
215-217: 🛠️ Refactor suggestionConsider utilizing the new validateSchedule method.
After unmarshalling the policy, you check if the schedule is nil but don't call the
validateSchedulemethod that was just added. Consider using it for comprehensive validation.if rPolicy.Schedule == nil { return fmt.Errorf("policy schedule is nil") } +if err := p.validateSchedule(rPolicy.Schedule); err != nil { + return fmt.Errorf("invalid schedule: %w", err) +}Also applies to: 155-172
🧹 Nitpick comments (3)
storage/postgres/time_trigger_test.go (2)
16-16: Consider removing or documenting the reason for skipping this test.The test is currently skipped by default, which means it won't run in CI/CD pipelines. If this is intentional (e.g., requires specific database setup), please add a comment explaining why and when it should be enabled.
+// Skip this test by default as it requires a running Postgres instance. +// To run: go test -v -run TestTimeTrigger ./storage/postgres t.SkipNow()
41-53: Consider using a cleaner approach for transaction rollback.The transaction rollback pattern could be simplified using defer to ensure cleanup even if the test panics.
ctx := context.Background() tx, err := backend.pool.Begin(ctx) if err != nil { t.Fatalf("Failed to begin transaction: %v", err) } +defer func() { + if err != nil { + tx.Rollback(ctx) + } +}() + if _, err := backend.InsertPluginPolicyTx(ctx, tx, vtypes.PluginPolicy{ ID: policyID, PublicKey: "4a7b9c2f8e1d3a5b6c4f9e2d7a1b3c5f8d4e6a2b9c1f7e3d5a6b4c2f8e1d3a5b", PluginID: "vultisig-payroll-0000", PluginVersion: "1", PolicyVersion: "1", Signature: "whatever", Recipe: "k3uaL47TpWtsT54tfhO7K6Gbyxf4H71jWma0zC2e3kQ=", Active: true, }); err != nil { - tx.Rollback(ctx) t.Fatalf("Failed to insert plugin policy: %v", err) }internal/scheduler/scheduler.go (1)
185-193: Remove unused struct definition.The
policySchedulestruct defined at the beginning ofGetTriggerFromPolicyis no longer used after the refactoring to protobuf types.func (s *SchedulerService) GetTriggerFromPolicy(policy vtypes.PluginPolicy) (*types.TimeTrigger, error) { - var policySchedule struct { - Schedule struct { - Frequency string `json:"frequency"` - StartTime time.Time `json:"start_time"` - Interval string `json:"interval"` - EndTime *time.Time `json:"end_time,omitempty"` - } `json:"schedule"` - } var p rtypes.Policy
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
api/server.go(1 hunks)cmd/payroll/worker/main.go(1 hunks)go.mod(1 hunks)internal/scheduler/scheduler.go(6 hunks)internal/types/time_trigger.go(2 hunks)plugin/payroll/policy.go(3 hunks)storage/postgres/db.go(3 hunks)storage/postgres/migrations/plugin/20241211182120_add_time_triggers.sql(1 hunks)storage/postgres/time_trigger_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
storage/postgres/db.go (2)
storage/postgres/db_system.go (1)
NewSystemMigrationManager(18-20)storage/postgres/db_plugin.go (1)
NewPluginMigrationManager(21-23)
storage/postgres/time_trigger_test.go (2)
storage/postgres/db.go (1)
NewPostgresBackend(28-52)internal/types/time_trigger.go (1)
TimeTrigger(17-26)
api/server.go (1)
internal/scheduler/scheduler.go (1)
NewSchedulerService(34-50)
🔇 Additional comments (11)
cmd/payroll/worker/main.go (1)
35-35: LGTM - Minor formatting improvement.The added blank line improves code readability by providing visual separation after the
redisOptionsinitialization.go.mod (1)
21-21: LGTM - Dependency update supports protobuf enum migration.The updated version of
github.com/vultisig/recipesprovides thertypes.ScheduleFrequencyenum type needed for the frequency field migration from string to protobuf enum.storage/postgres/migrations/plugin/20241211182120_add_time_triggers.sql (1)
14-14:Details
✅ Verification successful
Verify data migration strategy for existing frequency values.
The change from
TEXTtoINTfor the frequency column correctly supports the migration to protobuf enum values. However, ensure that any existing data in the frequency column is properly migrated or that this is a new table without existing data.
🏁 Script executed:
#!/bin/bash # Description: Check if this is a new table or if there's existing data that needs migration # Expected: Either no existing migration files or a data migration strategy # Check for existing time_triggers table usage rg -A 5 -B 5 "time_triggers" --type sqlLength of output: 2498
New
time_triggerstable: no data migration needed.This migration creates the
time_triggerstable from scratch—there’s no preexistingtime_triggersto convert, so you don’t need a data‐migration strategy for thefrequency(nowinterval) column.storage/postgres/db.go (1)
54-81: LGTM - Improved naming consistency.Renaming the receiver variable from
dtopin theCloseandMigratemethods improves consistency with thePostgresBackendstruct name. All references are updated correctly, and the minor formatting improvements enhance readability.internal/types/time_trigger.go (1)
7-7: LGTM - Required import for protobuf enum type.The import for
rtypesis correctly added to support theScheduleFrequencyenum type.storage/postgres/time_trigger_test.go (1)
31-31:Details
❌ Incorrect review comment
Verify that the frequency enum value is correctly represented in the database.
The test uses
rtypes.ScheduleFrequency_SCHEDULE_FREQUENCY_DAILYwhich should be stored as an integer in the database according to the migration. Ensure this aligns with the database schema change from TEXT to INT.Run the following script to verify the enum value mapping:
🏁 Script executed:
#!/bin/bash # Description: Verify the protobuf enum values for ScheduleFrequency # Search for the ScheduleFrequency enum definition ast-grep --pattern 'enum ScheduleFrequency { $$$ }' # Also check for any usage of the enum values to understand the mapping rg -A 3 'ScheduleFrequency_SCHEDULE_FREQUENCY'Length of output: 3774
#!/bin/bash # Search for ScheduleFrequency enum definition in .proto files rg -n "enum ScheduleFrequency" --context 3 --glob '*.proto' # Verify the numeric value assigned to SCHEDULE_FREQUENCY_DAILY rg -n "SCHEDULE_FREQUENCY_DAILY" --context 1 --glob '*.proto'
#!/bin/bash # Locate the protobuf definition to confirm numeric values rg -n "enum ScheduleFrequency" -g '*.proto' -A5 # Extract the numeric assignment for DAILY rg -n "SCHEDULE_FREQUENCY_DAILY" -g '*.proto' -A2 # Inspect how the test maps/scans the Frequency column rg -n "Frequency:" -C3 storage/postgres/time_trigger_test.go
🏁 Script executed:
#!/bin/bash # Locate protobuf enum definition for ScheduleFrequency rg -n "enum ScheduleFrequency" -g '*.proto' -A5 # Extract the numeric value for DAILY rg -n "SCHEDULE_FREQUENCY_DAILY" -g '*.proto' -A2 # Verify the frequency column type in the migration for time_triggers rg -n "time_triggers" -g '*.sql' -C3Length of output: 2338
No separate INT column for Frequency—cron_expression TEXT is used
Thetime_triggerstable stores the schedule in thecron_expressionTEXT column rather than a dedicatedfrequencyINT field. There is no direct enum-to-INT mapping in the migration, so the original request to verify an integer representation ofScheduleFrequency_SCHEDULE_FREQUENCY_DAILYdoes not apply.Likely an incorrect or invalid review comment.
api/server.go (1)
60-69: Good improvement in error handling for scheduler initialization.The change properly handles errors from
NewSchedulerServiceand prevents server startup if the scheduler fails to initialize. This is a significant improvement over the previous implementation.plugin/payroll/policy.go (1)
155-172: Well-structured schedule validation method.The validation logic is comprehensive and covers all essential checks including nil schedule, frequency validation, start time presence, and end time logic.
internal/scheduler/scheduler.go (3)
34-50: Excellent improvement in error handling.The change from using
logger.Fatalto returning an error is a significant improvement. This allows proper error propagation and gives callers the ability to handle initialization failures gracefully.
194-214: Good migration to protobuf-based schedule handling.The refactoring to use
rtypes.Policyand protobuf types improves type safety and consistency. The error handling and default value handling are well implemented.
421-424: Good handling of month overflow edge case.The logic correctly handles months with fewer days than the target day (e.g., scheduling for the 31st on months with 30 days). This is a thoughtful implementation detail.
Summary by CodeRabbit