diff --git a/pkg/workflow/config_helpers.go b/pkg/workflow/config_helpers.go index 83c6ec7057..1cb70b327e 100644 --- a/pkg/workflow/config_helpers.go +++ b/pkg/workflow/config_helpers.go @@ -181,6 +181,40 @@ func parseAllowedLabelsFromConfig(configMap map[string]any) []string { // to consolidate all time parsing logic in a single location. These functions are used // for parsing expiration configurations in safe output jobs. +// preprocessExpiresField handles the common expires field preprocessing pattern. +// This function: +// 1. Parses the expires value through parseExpiresFromConfig (handles integers, strings, and boolean false) +// 2. Handles explicit disablement when expires=false (returns -1) +// 3. Normalizes the value to hours and updates configData["expires"] in place +// 4. Logs the parsed value with the provided logger +// +// Returns true if expires was explicitly disabled with false, false otherwise. +// This helper consolidates duplicate preprocessing logic used in parseIssuesConfig and parseDiscussionsConfig. +func preprocessExpiresField(configData map[string]any, log *logger.Logger) bool { + expiresDisabled := false + if configData != nil { + if expires, exists := configData["expires"]; exists { + // Always parse the expires value through parseExpiresFromConfig + // This handles: integers (days), strings (time specs like "48h"), and boolean false + expiresInt := parseExpiresFromConfig(configData) + if expiresInt == -1 { + // Explicitly disabled with false + expiresDisabled = true + configData["expires"] = 0 + } else if expiresInt > 0 { + configData["expires"] = expiresInt + } else { + // Invalid or missing - set to 0 + configData["expires"] = 0 + } + if log != nil { + log.Printf("Parsed expires value %v to %d hours (disabled=%t)", expires, expiresInt, expiresDisabled) + } + } + } + return expiresDisabled +} + // ParseIntFromConfig is a generic helper that extracts and validates an integer value from a map. // Supports int, int64, float64, and uint64 types. // Returns the integer value, or 0 if not present or invalid. diff --git a/pkg/workflow/config_parsing_helpers_test.go b/pkg/workflow/config_parsing_helpers_test.go index fadc223f62..9c3562e15c 100644 --- a/pkg/workflow/config_parsing_helpers_test.go +++ b/pkg/workflow/config_parsing_helpers_test.go @@ -952,6 +952,124 @@ func TestParseBoolFromConfig(t *testing.T) { } } +func TestPreprocessExpiresField(t *testing.T) { + tests := []struct { + name string + input map[string]any + expectedDisabled bool + expectedValue int + }{ + { + name: "valid integer days - converted to hours", + input: map[string]any{ + "expires": 7, + }, + expectedDisabled: false, + expectedValue: 168, // 7 days * 24 hours + }, + { + name: "valid string format - 48h", + input: map[string]any{ + "expires": "48h", + }, + expectedDisabled: false, + expectedValue: 48, + }, + { + name: "valid string format - 7d", + input: map[string]any{ + "expires": "7d", + }, + expectedDisabled: false, + expectedValue: 168, + }, + { + name: "explicitly disabled with false", + input: map[string]any{ + "expires": false, + }, + expectedDisabled: true, + expectedValue: 0, + }, + { + name: "invalid - true boolean", + input: map[string]any{ + "expires": true, + }, + expectedDisabled: false, + expectedValue: 0, + }, + { + name: "invalid - 1 hour (below minimum)", + input: map[string]any{ + "expires": "1h", + }, + expectedDisabled: false, + expectedValue: 0, + }, + { + name: "valid - 2 hours (at minimum)", + input: map[string]any{ + "expires": "2h", + }, + expectedDisabled: false, + expectedValue: 2, + }, + { + name: "no expires field", + input: map[string]any{}, + expectedDisabled: false, + expectedValue: 0, // configData["expires"] not set when field missing + }, + { + name: "invalid string format", + input: map[string]any{ + "expires": "invalid", + }, + expectedDisabled: false, + expectedValue: 0, + }, + { + name: "nil configData", + input: nil, + expectedDisabled: false, + expectedValue: 0, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Make a copy of input to check modification + var configData map[string]any + if tt.input != nil { + configData = make(map[string]any) + for k, v := range tt.input { + configData[k] = v + } + } + + disabled := preprocessExpiresField(configData, nil) + + if disabled != tt.expectedDisabled { + t.Errorf("expected disabled=%v, got %v", tt.expectedDisabled, disabled) + } + + // Check that configData["expires"] was updated (if configData is not nil) + if configData != nil && tt.input != nil { + if _, exists := tt.input["expires"]; exists { + expiresValue, ok := configData["expires"].(int) + if !ok && configData["expires"] != nil { + t.Errorf("expected expires to be int, got %T", configData["expires"]) + } + if expiresValue != tt.expectedValue { + t.Errorf("expected configData[\"expires\"]=%d, got %d", tt.expectedValue, expiresValue) + } + } + } + }) + } +} + func TestUnmarshalConfig(t *testing.T) { tests := []struct { name string diff --git a/pkg/workflow/create_discussion.go b/pkg/workflow/create_discussion.go index 52610b4d09..4e73c449c5 100644 --- a/pkg/workflow/create_discussion.go +++ b/pkg/workflow/create_discussion.go @@ -38,25 +38,7 @@ func (c *Compiler) parseDiscussionsConfig(outputMap map[string]any) *CreateDiscu configData, _ := outputMap["create-discussion"].(map[string]any) // Pre-process the expires field (convert to hours before unmarshaling) - expiresDisabled := false - if configData != nil { - if expires, exists := configData["expires"]; exists { - // Always parse the expires value through parseExpiresFromConfig - // This handles: integers (days), strings (time specs like "48h"), and boolean false - expiresInt := parseExpiresFromConfig(configData) - if expiresInt == -1 { - // Explicitly disabled with false - expiresDisabled = true - configData["expires"] = 0 - } else if expiresInt > 0 { - configData["expires"] = expiresInt - } else { - // Invalid or missing - set to 0 - configData["expires"] = 0 - } - discussionLog.Printf("Parsed expires value %v to %d hours (disabled=%t)", expires, expiresInt, expiresDisabled) - } - } + expiresDisabled := preprocessExpiresField(configData, discussionLog) // Unmarshal into typed config struct var config CreateDiscussionsConfig diff --git a/pkg/workflow/create_issue.go b/pkg/workflow/create_issue.go index f296c5ac5f..184a6ef02e 100644 --- a/pkg/workflow/create_issue.go +++ b/pkg/workflow/create_issue.go @@ -35,25 +35,7 @@ func (c *Compiler) parseIssuesConfig(outputMap map[string]any) *CreateIssuesConf configData, _ := outputMap["create-issue"].(map[string]any) // Pre-process the expires field (convert to hours before unmarshaling) - expiresDisabled := false - if configData != nil { - if expires, exists := configData["expires"]; exists { - // Always parse the expires value through parseExpiresFromConfig - // This handles: integers (days), strings (time specs like "48h"), and boolean false - expiresInt := parseExpiresFromConfig(configData) - if expiresInt == -1 { - // Explicitly disabled with false - expiresDisabled = true - configData["expires"] = 0 - } else if expiresInt > 0 { - configData["expires"] = expiresInt - } else { - // Invalid or missing - set to 0 - configData["expires"] = 0 - } - createIssueLog.Printf("Parsed expires value %v to %d hours (disabled=%t)", expires, expiresInt, expiresDisabled) - } - } + expiresDisabled := preprocessExpiresField(configData, createIssueLog) // Unmarshal into typed config struct var config CreateIssuesConfig