Skip to content

Commit 7a4dea1

Browse files
committed
feat: add validation warnings for 5-field cron expressions
- Detect when users provide Unix 5-field cron format - Provide helpful warning with suggested 6-field conversion - Maintain backwards compatibility - expressions still work - Add comprehensive tests for validation function This helps users avoid confusion where '*/5 * * * *' (intended for every 5 minutes) actually runs every 5 seconds due to the missing seconds field.
1 parent 99b56f9 commit 7a4dea1

File tree

2 files changed

+100
-5
lines changed

2 files changed

+100
-5
lines changed

provider/script.go

Lines changed: 28 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package provider
33
import (
44
"context"
55
"fmt"
6+
"strings"
67

78
"github.com/google/uuid"
89
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
@@ -13,6 +14,32 @@ import (
1314

1415
var ScriptCRONParser = cron.NewParser(cron.Second | cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.DowOptional | cron.Descriptor)
1516

17+
// ValidateCronExpression validates a cron expression and provides helpful warnings for common mistakes
18+
func ValidateCronExpression(cronExpr string) (warnings []string, errors []error) {
19+
// Check if it looks like a 5-field Unix cron expression
20+
fields := strings.Fields(cronExpr)
21+
if len(fields) == 5 {
22+
// Try to parse as standard Unix cron (without seconds)
23+
unixParser := cron.NewParser(cron.Minute | cron.Hour | cron.Dom | cron.Month | cron.DowOptional | cron.Descriptor)
24+
if _, err := unixParser.Parse(cronExpr); err == nil {
25+
// It's a valid 5-field expression, provide a helpful warning
26+
warnings = append(warnings, fmt.Sprintf(
27+
"The cron expression '%s' appears to be in Unix 5-field format. "+
28+
"Coder uses 6-field format (seconds minutes hours day month day-of-week). "+
29+
"Consider prefixing with '0 ' to run at the start of each minute: '0 %s'",
30+
cronExpr, cronExpr))
31+
}
32+
}
33+
34+
// Validate with the actual 6-field parser
35+
_, err := ScriptCRONParser.Parse(cronExpr)
36+
if err != nil {
37+
errors = append(errors, fmt.Errorf("%s is not a valid cron expression: %w", cronExpr, err))
38+
}
39+
40+
return warnings, errors
41+
}
42+
1643
func scriptResource() *schema.Resource {
1744
return &schema.Resource{
1845
SchemaVersion: 1,
@@ -78,11 +105,7 @@ func scriptResource() *schema.Resource {
78105
if !ok {
79106
return []string{}, []error{fmt.Errorf("got type %T instead of string", i)}
80107
}
81-
_, err := ScriptCRONParser.Parse(v)
82-
if err != nil {
83-
return []string{}, []error{fmt.Errorf("%s is not a valid cron expression: %w", v, err)}
84-
}
85-
return nil, nil
108+
return ValidateCronExpression(v)
86109
},
87110
},
88111
"start_blocks_login": {

provider/script_test.go

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,8 @@ import (
88

99
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
1010
"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
11+
12+
"github.com/coder/terraform-provider-coder/v2/provider"
1113
)
1214

1315
func TestScript(t *testing.T) {
@@ -124,3 +126,73 @@ func TestScriptStartBlocksLoginRequiresRunOnStart(t *testing.T) {
124126
}},
125127
})
126128
}
129+
130+
func TestValidateCronExpression(t *testing.T) {
131+
t.Parallel()
132+
133+
tests := []struct {
134+
name string
135+
cronExpr string
136+
expectWarnings bool
137+
expectErrors bool
138+
warningContains string
139+
}{
140+
{
141+
name: "valid 6-field expression",
142+
cronExpr: "0 0 22 * * *",
143+
expectWarnings: false,
144+
expectErrors: false,
145+
},
146+
{
147+
name: "valid 6-field expression with seconds",
148+
cronExpr: "30 0 9 * * 1-5",
149+
expectWarnings: false,
150+
expectErrors: false,
151+
},
152+
{
153+
name: "5-field Unix format - should warn",
154+
cronExpr: "0 22 * * *",
155+
expectWarnings: true,
156+
expectErrors: false,
157+
warningContains: "appears to be in Unix 5-field format",
158+
},
159+
{
160+
name: "5-field every 5 minutes - should warn",
161+
cronExpr: "*/5 * * * *",
162+
expectWarnings: true,
163+
expectErrors: false,
164+
warningContains: "Consider prefixing with '0 '",
165+
},
166+
{
167+
name: "invalid expression",
168+
cronExpr: "invalid",
169+
expectErrors: true,
170+
},
171+
{
172+
name: "too many fields",
173+
cronExpr: "0 0 0 0 0 0 0",
174+
expectErrors: true,
175+
},
176+
}
177+
178+
for _, tt := range tests {
179+
t.Run(tt.name, func(t *testing.T) {
180+
warnings, errors := provider.ValidateCronExpression(tt.cronExpr)
181+
182+
if tt.expectWarnings {
183+
require.NotEmpty(t, warnings, "Expected warnings but got none")
184+
if tt.warningContains != "" {
185+
require.Contains(t, warnings[0], tt.warningContains)
186+
}
187+
} else {
188+
require.Empty(t, warnings, "Expected no warnings but got: %v", warnings)
189+
}
190+
191+
if tt.expectErrors {
192+
require.NotEmpty(t, errors, "Expected errors but got none")
193+
} else {
194+
require.Empty(t, errors, "Expected no errors but got: %v", errors)
195+
}
196+
})
197+
}
198+
}

0 commit comments

Comments
 (0)