-
Notifications
You must be signed in to change notification settings - Fork 528
Enforce explicit job and step timeouts on Visual Regression Checker workflow #56980
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
17d09fe
1912044
9775208
6f5bb95
289029e
d7c32e9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import ( | |
| "errors" | ||
| "fmt" | ||
| "maps" | ||
| "math" | ||
| "strconv" | ||
|
|
||
| "github.com/github/gh-aw/pkg/importinpututil" | ||
|
|
@@ -117,34 +118,13 @@ func MapToStep(stepMap map[string]any) (*WorkflowStep, error) { | |
| step.With = with | ||
| } | ||
| if env, ok := stepMap["env"].(map[string]any); ok { | ||
| // Convert map[string]any to map[string]string | ||
| step.Env = make(map[string]string) | ||
| for k, v := range env { | ||
| if strVal, ok := v.(string); ok { | ||
| step.Env[k] = strVal | ||
| } else if v != nil { | ||
| // Arrays and maps are serialized as JSON so that shell consumers | ||
| // (e.g. jq --argjson) receive valid JSON. This handles both the | ||
| // []any / map[string]any case returned by encoding/json and the | ||
| // typed-slice case (e.g. []string) returned by goccy/go-yaml. | ||
| step.Env[k] = marshalEnvValue(v) | ||
| } | ||
| } | ||
| step.Env = parseStepEnv(env) | ||
| } | ||
| if continueOnError, ok := stepMap["continue-on-error"]; ok { | ||
| switch value := continueOnError.(type) { | ||
| case bool: | ||
| templatableValue := TemplatableBool(strconv.FormatBool(value)) | ||
| step.ContinueOnError = &templatableValue | ||
| case string: | ||
| if value == "true" || value == "false" || isExpression(value) { | ||
| templatableValue := TemplatableBool(value) | ||
| step.ContinueOnError = &templatableValue | ||
| } | ||
| } | ||
| step.ContinueOnError = parseStepContinueOnError(continueOnError) | ||
| } | ||
| if timeoutMinutes, ok := stepMap["timeout-minutes"].(int); ok { | ||
| step.TimeoutMinutes = timeoutMinutes | ||
| if timeoutMinutesVal, ok := stepMap["timeout-minutes"]; ok { | ||
| step.TimeoutMinutes = parseStepTimeoutMinutes(timeoutMinutesVal) | ||
| } | ||
|
|
||
| stepType := "unknown" | ||
|
|
@@ -157,6 +137,56 @@ func MapToStep(stepMap map[string]any) (*WorkflowStep, error) { | |
| return step, nil | ||
| } | ||
|
|
||
| func parseStepEnv(env map[string]any) map[string]string { | ||
| result := make(map[string]string) | ||
| for k, v := range env { | ||
| if strVal, ok := v.(string); ok { | ||
| result[k] = strVal | ||
| } else if v != nil { | ||
| result[k] = marshalEnvValue(v) | ||
| } | ||
| } | ||
| return result | ||
| } | ||
|
|
||
| func parseStepContinueOnError(val any) *TemplatableBool { | ||
| switch value := val.(type) { | ||
| case bool: | ||
| templatableValue := TemplatableBool(strconv.FormatBool(value)) | ||
| return &templatableValue | ||
| case string: | ||
| if value == "true" || value == "false" || isExpression(value) { | ||
| templatableValue := TemplatableBool(value) | ||
| return &templatableValue | ||
| } | ||
| } | ||
| return nil | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The 💡 Suggested fix + missing testcase int:
if v > 0 {
return v
}The A regression test covering invalid inputs (negative ints, negative strings, zero) would prevent this from regressing. @copilot please address this. |
||
| } | ||
|
|
||
| func parseStepTimeoutMinutes(val any) int { | ||
| switch v := val.(type) { | ||
| case int: | ||
| return v | ||
| case int64: | ||
| if v > 0 && v <= int64(math.MaxInt) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Suggested fix: case int:
if v > 0 {
return v
}@copilot please address this. |
||
| return int(v) | ||
| } | ||
| case uint64: | ||
| if v <= uint64(math.MaxInt) { | ||
| return int(v) | ||
| } | ||
| case float64: | ||
| if v > 0 && v <= float64(math.MaxInt) { | ||
| return int(v) | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Suggested fix: case string:
if n, err := strconv.Atoi(v); err == nil && n > 0 {
return n
}@copilot please address this.
Comment on lines
+183
to
+192
|
||
| case string: | ||
| if n, err := strconv.Atoi(v); err == nil { | ||
| return n | ||
| } | ||
| } | ||
| return 0 | ||
| } | ||
|
|
||
| // Clone creates a deep copy of the WorkflowStep | ||
| func (s *WorkflowStep) Clone() *WorkflowStep { | ||
| clone := &WorkflowStep{ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -663,6 +663,33 @@ func TestSliceToSteps_RoundTrip(t *testing.T) { | |
| } | ||
| } | ||
|
|
||
| func TestMapToStep_TimeoutMinutesNumericTypes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
| val any | ||
| want int | ||
| }{ | ||
| {"int", 5, 5}, | ||
| {"int64", int64(10), 10}, | ||
| {"uint64", uint64(15), 15}, | ||
| {"float64", float64(20), 20}, | ||
| {"string", "25", 25}, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| stepMap := map[string]any{ | ||
| "name": "Test", | ||
| "run": "echo test", | ||
| "timeout-minutes": tt.val, | ||
| } | ||
| step, err := MapToStep(stepMap) | ||
| require.NoError(t, err) | ||
| assert.Equal(t, tt.want, step.TimeoutMinutes) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The new test 💡 Suggested additional test cases{"negative int", -1, 0},
{"zero", 0, 0},
{"negative float", float64(-3), 0},
{"negative string", "-5", 0},
{"bool (unsupported)", true, 0},Having these in the table would have caught the missing lower-bound guard before the PR was submitted. @copilot please address this. |
||
| }) | ||
| } | ||
| } | ||
|
|
||
| func TestMapToStep_InvalidTypes(t *testing.T) { | ||
| tests := []struct { | ||
| name string | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pkg/workflow/step_types.go:140: yagni: three one-caller parsing helpers for env/continue-on-error/timeout. Inline the small switch logic in MapToStep and keep the mapping local.