Skip to content

fix(cli): improve schedule trigger schema#141

Merged
jyecusch merged 5 commits intomainfrom
fix/schedules-schema
Nov 3, 2025
Merged

fix(cli): improve schedule trigger schema#141
jyecusch merged 5 commits intomainfrom
fix/schedules-schema

Conversation

@jyecusch
Copy link
Member

@jyecusch jyecusch commented Nov 3, 2025

Reduce configuration depth from:

services:
  api:
    triggers:
      poll:
        path: /schedule
        schedule:
          cron_expression: "*/5 * * * *"

To:

services:
  api:
    schedules:
      - path: /schedule2
        cron: "*/5 * * * *"
      - path: /schedule
        cron: "*/1 * * * *"

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 3, 2025

📝 Walkthrough

Walkthrough

Service intent replaces `Triggers` (map) with `Schedules` (slice); each Schedule exposes a direct `Cron` string and `Path`. Code was updated to iterate `Schedules`, trim cron whitespace, error on empty crons, register only non-empty crons, and use `schedule.Path` for URL construction. Tests, CLI service code, and Terraform deployment were adjusted accordingly; cron execution logging was modified.

Pre-merge checks

✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "fix(cli): improve schedule trigger schema" directly describes the main objective of the changeset, which is to simplify the configuration structure for schedule triggers by flattening the nested schema. The title is concise, specific, and uses conventional commit formatting without vague terms or noise. It clearly communicates the primary change across all modified files in the pull request.
Description Check ✅ Passed The description is directly related to the changeset, providing concrete before/after YAML examples that illustrate the exact configuration transformation being implemented. The examples show the shift from a deeply nested structure (triggers → poll → schedule → cron_expression) to a flatter list-based structure (schedules with direct cron and path fields), which aligns with the schema and service changes documented in the raw summary.

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cli/internal/simulation/service/service.go (1)

128-161: Isolate cron handler state per trigger

triggerName, trigger, cronExpression, and url are loop variables captured by reference, so every scheduled fire prints and hits the last trigger only. Copy them into locals before calling AddFunc.

-		cronExpression := strings.TrimSpace(trigger.Cron)
+		cronExpression := strings.TrimSpace(trigger.Cron)
 
 		if cronExpression != "" {
-			url := url.URL{
+			nameCopy := triggerName
+			pathCopy := trigger.Path
+			exprCopy := cronExpression
+			targetURL := url.URL{
 				Scheme: "http",
 				Host:   fmt.Sprintf("localhost:%d", s.port),
-				Path:   trigger.Path,
+				Path:   pathCopy,
 			}
+			urlStr := targetURL.String()
 
-			_, err := cron.AddFunc(cronExpression, func() {
-				fmt.Printf("%s triggering %s POST %s\n", style.Purple(fmt.Sprintf("[%s.%s] cron:%s", s.name, triggerName, cronExpression)), style.Teal(fmt.Sprintf("[%s]", s.name)), trigger.Path)
-				req, err := http.NewRequest(http.MethodPost, url.String(), nil)
+			_, err := cron.AddFunc(exprCopy, func() {
+				fmt.Printf("%s triggering %s POST %s\n", style.Purple(fmt.Sprintf("[%s.%s] cron:%s", s.name, nameCopy, exprCopy)), style.Teal(fmt.Sprintf("[%s]", s.name)), pathCopy)
+				req, err := http.NewRequest(http.MethodPost, urlStr, nil)
 				if err != nil {
 					// log the error
 					fmt.Fprint(stderrorWriter, "error creating request for schedule", err)
 					return
 				}
 
 				resp, err := http.DefaultClient.Do(req)
 				if err != nil {
 					fmt.Fprint(stderrorWriter, "error sending request for schedule", err)
 					return
 				}
 
 				defer resp.Body.Close()
 
 				if resp.StatusCode != http.StatusOK {
 					fmt.Fprint(stderrorWriter, "error sending request for schedule", resp.StatusCode)
 					return
 				}
 
-				fmt.Fprintf(stdoutWriter, "schedule [%s] triggered on %s", triggerName, trigger.Path)
+				fmt.Fprintf(stdoutWriter, "schedule [%s] triggered on %s", nameCopy, pathCopy)
 			})
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d8984ad and 5d15afc.

📒 Files selected for processing (4)
  • cli/internal/simulation/service/service.go (2 hunks)
  • cli/pkg/schema/schema_test.go (2 hunks)
  • cli/pkg/schema/service.go (1 hunks)
  • engines/terraform/deployment.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
engines/terraform/deployment.go (1)
engines/terraform/suga.go (1)
  • SugaServiceSchedule (16-19)
cli/internal/simulation/service/service.go (2)
cli/internal/style/style.go (2)
  • Purple (18-18)
  • Teal (16-16)
cli/internal/style/colors/colors.go (2)
  • Purple (21-21)
  • Teal (19-19)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 154c4b3 and 75a38aa.

📒 Files selected for processing (1)
  • cli/internal/simulation/service/service.go (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
cli/internal/simulation/service/service.go (2)
cli/internal/style/style.go (2)
  • Teal (16-16)
  • Purple (18-18)
cli/internal/style/colors/colors.go (2)
  • Teal (19-19)
  • Purple (21-21)
⏰ 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). (4)
  • GitHub Check: Build (macos-latest, arm64)
  • GitHub Check: Build (windows-latest, amd64)
  • GitHub Check: Security Scan
  • GitHub Check: Test

@jyecusch jyecusch merged commit 772cb70 into main Nov 3, 2025
12 checks passed
@jyecusch jyecusch deleted the fix/schedules-schema branch November 3, 2025 03:11
@nitric-bot
Copy link

🎉 This PR is included in version 0.4.3 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants