From 9faf61a28f31445155b65e32b5a51d75c3e3851e Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 Aug 2025 05:43:41 +0000 Subject: [PATCH 1/4] Initial plan From 67cbdf9dba07208af0fd458d5e3d2193d5581db6 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 Aug 2025 06:08:54 +0000 Subject: [PATCH 2/4] Implement sophisticated concurrency policy computation system Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/schemas/main_workflow_schema.json | 27 + pkg/workflow/compiler.go | 8 +- pkg/workflow/concurrency.go | 68 ++- pkg/workflow/concurrency_policy.go | 390 ++++++++++++++ .../concurrency_policy_integration_test.go | 275 ++++++++++ pkg/workflow/concurrency_policy_test.go | 499 ++++++++++++++++++ 6 files changed, 1261 insertions(+), 6 deletions(-) create mode 100644 pkg/workflow/concurrency_policy.go create mode 100644 pkg/workflow/concurrency_policy_integration_test.go create mode 100644 pkg/workflow/concurrency_policy_test.go diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 809fa8d61e0..af51ac9ea42 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -223,6 +223,33 @@ } ] }, + "concurrency_policy": { + "type": "object", + "description": "Concurrency policy configuration for different workflow trigger types", + "additionalProperties": { + "type": "object", + "description": "Policy configuration for a specific trigger type", + "properties": { + "group": { + "type": "string", + "description": "Group identifier for concurrency (defaults to 'workflow')" + }, + "id": { + "type": "string", + "description": "Legacy group identifier (use 'group' instead)" + }, + "node": { + "type": "string", + "description": "Node expression for dynamic concurrency (e.g., 'issue.number', 'pull_request.number', 'github.ref')" + }, + "cancel-in-progress": { + "type": "boolean", + "description": "Whether to cancel in-progress workflows when new ones start" + } + }, + "additionalProperties": false + } + }, "env": { "description": "Environment variables for the workflow", "oneOf": [ diff --git a/pkg/workflow/compiler.go b/pkg/workflow/compiler.go index ce84ed76c78..5c68d77576d 100644 --- a/pkg/workflow/compiler.go +++ b/pkg/workflow/compiler.go @@ -721,7 +721,7 @@ func (c *Compiler) parseWorkflowFile(markdownPath string) (*WorkflowData, error) } // Apply defaults - c.applyDefaults(workflowData, markdownPath) + c.applyDefaults(workflowData, markdownPath, result.Frontmatter) // Apply pull request draft filter if specified c.applyPullRequestDraftFilter(workflowData, result.Frontmatter) @@ -881,7 +881,7 @@ func (c *Compiler) extractAliasName(frontmatter map[string]any) string { } // applyDefaults applies default values for missing workflow sections -func (c *Compiler) applyDefaults(data *WorkflowData, markdownPath string) { +func (c *Compiler) applyDefaults(data *WorkflowData, markdownPath string, frontmatter map[string]any) { // Check if this is an alias trigger workflow (by checking if user specified "on.alias") isAliasTrigger := false if data.On == "" { @@ -988,8 +988,8 @@ func (c *Compiler) applyDefaults(data *WorkflowData, markdownPath string) { models: read` } - // Generate concurrency configuration using the dedicated concurrency module - data.Concurrency = GenerateConcurrencyConfig(data, isAliasTrigger) + // Generate concurrency configuration using the new policy system + data.Concurrency = GenerateConcurrencyConfigWithFrontmatter(data, isAliasTrigger, frontmatter, c.verbose) if data.RunName == "" { data.RunName = fmt.Sprintf(`run-name: "%s"`, data.Name) diff --git a/pkg/workflow/concurrency.go b/pkg/workflow/concurrency.go index 0809f124501..55ec4f4e1b8 100644 --- a/pkg/workflow/concurrency.go +++ b/pkg/workflow/concurrency.go @@ -1,17 +1,81 @@ package workflow import ( + "fmt" "strings" + + "github.com/githubnext/gh-aw/pkg/console" ) // GenerateConcurrencyConfig generates the concurrency configuration for a workflow -// based on its trigger types and characteristics. +// based on its trigger types and characteristics. Now supports advanced policy computation. func GenerateConcurrencyConfig(workflowData *WorkflowData, isAliasTrigger bool) string { - // Don't override if already set + // Don't override if already set by user if workflowData.Concurrency != "" { return workflowData.Concurrency } + // Try to use the new policy system first + return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) +} + +// GenerateConcurrencyConfigWithFrontmatter generates concurrency config using the policy system +// This is the new entry point that accepts frontmatter for policy parsing +func GenerateConcurrencyConfigWithFrontmatter(workflowData *WorkflowData, isAliasTrigger bool, frontmatter map[string]any, verbose bool) string { + // Don't override if already set by user + if workflowData.Concurrency != "" { + return workflowData.Concurrency + } + + // Parse concurrency policy from frontmatter + userPolicySet, err := parseConcurrencyPolicyFromFrontmatter(frontmatter) + if err != nil { + if verbose { + fmt.Println(console.FormatWarningMessage("Failed to parse concurrency policy, using defaults: " + err.Error())) + } + // Fall back to legacy behavior + return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) + } + + // Compute the final policy + computed, err := computeConcurrencyPolicy(workflowData, isAliasTrigger, userPolicySet) + if err != nil { + if verbose { + fmt.Println(console.FormatWarningMessage("Failed to compute concurrency policy, using defaults: " + err.Error())) + } + // Fall back to legacy behavior + return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) + } + + // Generate YAML + yaml := generateConcurrencyYAML(computed) + if yaml == "" { + // Fall back to legacy behavior + return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) + } + + return yaml +} + +// generateConcurrencyWithPolicySystem uses the new policy system but with default policies only +func generateConcurrencyWithPolicySystem(workflowData *WorkflowData, isAliasTrigger bool) string { + // Compute policy using defaults only (no user override) + computed, err := computeConcurrencyPolicy(workflowData, isAliasTrigger, nil) + if err != nil { + // Fall back to legacy behavior if policy system fails + return generateLegacyConcurrency(workflowData, isAliasTrigger) + } + + yaml := generateConcurrencyYAML(computed) + if yaml == "" { + return generateLegacyConcurrency(workflowData, isAliasTrigger) + } + + return yaml +} + +// generateLegacyConcurrency provides the original concurrency generation logic as fallback +func generateLegacyConcurrency(workflowData *WorkflowData, isAliasTrigger bool) string { // Generate concurrency configuration based on workflow type // Note: Check alias trigger first since alias workflows also contain pull_request events if isAliasTrigger { diff --git a/pkg/workflow/concurrency_policy.go b/pkg/workflow/concurrency_policy.go new file mode 100644 index 00000000000..b8ab9e83d6f --- /dev/null +++ b/pkg/workflow/concurrency_policy.go @@ -0,0 +1,390 @@ +package workflow + +import ( + "fmt" + "strings" +) + +// ConcurrencyPolicy represents a single concurrency policy definition +type ConcurrencyPolicy struct { + Group string `json:"group" yaml:"group"` + Node string `json:"node" yaml:"node"` + CancelInProgress *bool `json:"cancel-in-progress,omitempty" yaml:"cancel-in-progress,omitempty"` +} + +// ConcurrencyPolicySet represents a set of policies for different contexts +type ConcurrencyPolicySet struct { + Default *ConcurrencyPolicy `json:"*,omitempty" yaml:"*,omitempty"` + Issues *ConcurrencyPolicy `json:"issues,omitempty" yaml:"issues,omitempty"` + PullRequest *ConcurrencyPolicy `json:"pull_requests,omitempty" yaml:"pull_requests,omitempty"` + Schedule *ConcurrencyPolicy `json:"schedule,omitempty" yaml:"schedule,omitempty"` + Manual *ConcurrencyPolicy `json:"workflow_dispatch,omitempty" yaml:"workflow_dispatch,omitempty"` + Custom map[string]*ConcurrencyPolicy `json:"-" yaml:"-"` // for any other trigger types +} + +// ComputedConcurrencyPolicy represents the final computed concurrency configuration +type ComputedConcurrencyPolicy struct { + Group string + CancelInProgress *bool +} + +// parseConcurrencyPolicyFromFrontmatter extracts concurrency policy configuration from frontmatter +func parseConcurrencyPolicyFromFrontmatter(frontmatter map[string]any) (*ConcurrencyPolicySet, error) { + // Look for "concurrency_policy" key in frontmatter + policyValue, exists := frontmatter["concurrency_policy"] + if !exists { + return nil, nil // No policy defined, use defaults + } + + policyMap, ok := policyValue.(map[string]any) + if !ok { + return nil, fmt.Errorf("concurrency_policy must be an object") + } + + policySet := &ConcurrencyPolicySet{ + Custom: make(map[string]*ConcurrencyPolicy), + } + + // Parse each policy context + for key, value := range policyMap { + valueMap, ok := value.(map[string]any) + if !ok { + return nil, fmt.Errorf("concurrency_policy.%s must be an object", key) + } + + policy, err := parseSinglePolicy(valueMap) + if err != nil { + return nil, fmt.Errorf("invalid concurrency_policy.%s: %w", key, err) + } + + // Assign to the appropriate field based on key + switch key { + case "*": + policySet.Default = policy + case "issues": + policySet.Issues = policy + case "pull_requests": + policySet.PullRequest = policy + case "schedule": + policySet.Schedule = policy + case "workflow_dispatch": + policySet.Manual = policy + default: + policySet.Custom[key] = policy + } + } + + return policySet, nil +} + +// parseSinglePolicy parses a single policy definition +func parseSinglePolicy(policyMap map[string]any) (*ConcurrencyPolicy, error) { + policy := &ConcurrencyPolicy{} + + // Parse group (can be either "group" or "id" for backwards compatibility) + if group, exists := policyMap["group"]; exists { + if groupStr, ok := group.(string); ok { + policy.Group = groupStr + } else { + return nil, fmt.Errorf("group must be a string") + } + } else if id, exists := policyMap["id"]; exists { + // Support "id" for backwards compatibility (mentioned in issue) + if idStr, ok := id.(string); ok { + policy.Group = idStr + } else { + return nil, fmt.Errorf("id must be a string") + } + } + + // Parse node + if node, exists := policyMap["node"]; exists { + if nodeStr, ok := node.(string); ok { + policy.Node = nodeStr + } else { + return nil, fmt.Errorf("node must be a string") + } + } + + // Parse cancel-in-progress + if cancel, exists := policyMap["cancel-in-progress"]; exists { + if cancelBool, ok := cancel.(bool); ok { + policy.CancelInProgress = &cancelBool + } else { + return nil, fmt.Errorf("cancel-in-progress must be a boolean") + } + } + + return policy, nil +} + +// computeConcurrencyPolicy merges policies from different sources and computes the final configuration +func computeConcurrencyPolicy(workflowData *WorkflowData, isAliasTrigger bool, userPolicySet *ConcurrencyPolicySet) (*ComputedConcurrencyPolicy, error) { + // Start with default policies based on workflow characteristics + defaultPolicySet := getDefaultPolicySet(isAliasTrigger) + + // Merge user-defined policies if provided + finalPolicySet := mergePolicySets(defaultPolicySet, userPolicySet) + + // Determine which specific policy to use based on workflow triggers + selectedPolicy := selectPolicyForWorkflow(workflowData, isAliasTrigger, finalPolicySet) + + // Compute the final concurrency configuration + computed := &ComputedConcurrencyPolicy{} + + if selectedPolicy != nil { + // Build the group identifier + computed.Group = buildGroupIdentifier(selectedPolicy, workflowData) + computed.CancelInProgress = selectedPolicy.CancelInProgress + } else { + // Fallback to basic group + computed.Group = "gh-aw-${{ github.workflow }}" + } + + return computed, nil +} + +// getDefaultPolicySet returns the default policy set based on workflow characteristics +func getDefaultPolicySet(isAliasTrigger bool) *ConcurrencyPolicySet { + policySet := &ConcurrencyPolicySet{ + Custom: make(map[string]*ConcurrencyPolicy), + } + + // Default policy for all workflows + policySet.Default = &ConcurrencyPolicy{ + Group: "workflow", + Node: "", + } + + // Issues policy with cancel + cancelTrue := true + policySet.Issues = &ConcurrencyPolicy{ + Group: "workflow", + Node: "issue.number || github.event.pull_request.number", // Support both issue and PR for alias + CancelInProgress: &cancelTrue, + } + + // Pull request policy with cancel (use ref for backwards compatibility with existing tests) + policySet.PullRequest = &ConcurrencyPolicy{ + Group: "workflow", + Node: "github.ref", // Use ref instead of pull_request.number for compatibility + CancelInProgress: &cancelTrue, + } + + // For alias triggers, override to not use cancellation + if isAliasTrigger { + policySet.Issues.CancelInProgress = nil + policySet.PullRequest.CancelInProgress = nil + } + + return policySet +} + +// mergePolicySets merges user-defined policies with default policies +func mergePolicySets(base, override *ConcurrencyPolicySet) *ConcurrencyPolicySet { + if override == nil { + return base + } + + result := &ConcurrencyPolicySet{ + Custom: make(map[string]*ConcurrencyPolicy), + } + + // Copy base policies + if base.Default != nil { + result.Default = copyPolicy(base.Default) + } + if base.Issues != nil { + result.Issues = copyPolicy(base.Issues) + } + if base.PullRequest != nil { + result.PullRequest = copyPolicy(base.PullRequest) + } + if base.Schedule != nil { + result.Schedule = copyPolicy(base.Schedule) + } + if base.Manual != nil { + result.Manual = copyPolicy(base.Manual) + } + for k, v := range base.Custom { + result.Custom[k] = copyPolicy(v) + } + + // Override with user-defined policies + if override.Default != nil { + result.Default = mergePolicy(result.Default, override.Default) + } + if override.Issues != nil { + result.Issues = mergePolicy(result.Issues, override.Issues) + } + if override.PullRequest != nil { + result.PullRequest = mergePolicy(result.PullRequest, override.PullRequest) + } + if override.Schedule != nil { + result.Schedule = mergePolicy(result.Schedule, override.Schedule) + } + if override.Manual != nil { + result.Manual = mergePolicy(result.Manual, override.Manual) + } + for k, v := range override.Custom { + result.Custom[k] = mergePolicy(result.Custom[k], v) + } + + return result +} + +// mergePolicy merges two policies, with override taking precedence +func mergePolicy(base, override *ConcurrencyPolicy) *ConcurrencyPolicy { + if override == nil { + return base + } + if base == nil { + return copyPolicy(override) + } + + result := copyPolicy(base) + + // Override fields if specified + if override.Group != "" { + result.Group = override.Group + } + if override.Node != "" { + result.Node = override.Node + } + if override.CancelInProgress != nil { + result.CancelInProgress = override.CancelInProgress + } + + return result +} + +// copyPolicy creates a deep copy of a policy +func copyPolicy(policy *ConcurrencyPolicy) *ConcurrencyPolicy { + if policy == nil { + return nil + } + + result := &ConcurrencyPolicy{ + Group: policy.Group, + Node: policy.Node, + } + + if policy.CancelInProgress != nil { + cancel := *policy.CancelInProgress + result.CancelInProgress = &cancel + } + + return result +} + +// selectPolicyForWorkflow selects the most appropriate policy for the given workflow +func selectPolicyForWorkflow(workflowData *WorkflowData, isAliasTrigger bool, policySet *ConcurrencyPolicySet) *ConcurrencyPolicy { + if isAliasTrigger { + // For alias workflows, prefer issues policy if available + if policySet.Issues != nil { + return policySet.Issues + } + } + + // Check if this is a pull request workflow + if isPullRequestWorkflow(workflowData.On) { + if policySet.PullRequest != nil { + return policySet.PullRequest + } + } + + // Check for schedule workflows + if strings.Contains(workflowData.On, "schedule") { + if policySet.Schedule != nil { + return policySet.Schedule + } + } + + // Check for manual workflows + if strings.Contains(workflowData.On, "workflow_dispatch") { + if policySet.Manual != nil { + return policySet.Manual + } + } + + // Check for issues workflows + if strings.Contains(workflowData.On, "issues") { + if policySet.Issues != nil { + return policySet.Issues + } + } + + // Check for custom trigger types in the policy custom map + for triggerType, customPolicy := range policySet.Custom { + if strings.Contains(workflowData.On, triggerType) { + return customPolicy + } + } + + // Fall back to default policy + return policySet.Default +} + +// buildGroupIdentifier constructs the final group identifier string +func buildGroupIdentifier(policy *ConcurrencyPolicy, workflowData *WorkflowData) string { + if policy == nil { + return "gh-aw-${{ github.workflow }}" + } + + // Start with the base group + var parts []string + + // Always include the gh-aw prefix + parts = append(parts, "gh-aw") + + // Add the workflow identifier + if policy.Group == "workflow" { + parts = append(parts, "${{ github.workflow }}") + } else { + // Use custom group identifier + parts = append(parts, policy.Group) + } + + // Add the node identifier if specified + if policy.Node != "" { + var nodeExpr string + switch policy.Node { + case "issue.number": + nodeExpr = "${{ github.event.issue.number }}" + case "pull_request.number": + nodeExpr = "${{ github.event.pull_request.number }}" + case "github.ref": + nodeExpr = "${{ github.ref }}" + case "issue.number || github.event.pull_request.number": + // Special case for alias workflows + nodeExpr = "${{ github.event.issue.number || github.event.pull_request.number }}" + default: + // Custom node expression + if strings.HasPrefix(policy.Node, "${{") { + nodeExpr = policy.Node + } else { + nodeExpr = fmt.Sprintf("${{ %s }}", policy.Node) + } + } + parts = append(parts, nodeExpr) + } + + return strings.Join(parts, "-") +} + +// generateConcurrencyYAML generates the final YAML for the concurrency section +func generateConcurrencyYAML(computed *ComputedConcurrencyPolicy) string { + if computed == nil { + return "" + } + + var lines []string + lines = append(lines, "concurrency:") + lines = append(lines, fmt.Sprintf(" group: \"%s\"", computed.Group)) + + if computed.CancelInProgress != nil && *computed.CancelInProgress { + lines = append(lines, " cancel-in-progress: true") + } + + return strings.Join(lines, "\n") +} diff --git a/pkg/workflow/concurrency_policy_integration_test.go b/pkg/workflow/concurrency_policy_integration_test.go new file mode 100644 index 00000000000..70799d6d179 --- /dev/null +++ b/pkg/workflow/concurrency_policy_integration_test.go @@ -0,0 +1,275 @@ +package workflow + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestConcurrencyPolicyIntegration(t *testing.T) { + // Test the new concurrency policy system with real workflow files + tmpDir, err := os.MkdirTemp("", "concurrency-policy-integration-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + compiler := NewCompiler(false, "", "test") + + tests := []struct { + name string + frontmatter string + filename string + expectedGroup string + expectedCancel *bool + description string + }{ + { + name: "workflow with custom concurrency policy", + frontmatter: `--- +name: custom-concurrency-test +on: + push: + branches: [main] +concurrency_policy: + "*": + group: custom-workflow + cancel-in-progress: true + pull_requests: + group: pr-specific + node: pull_request.number + cancel-in-progress: false +---`, + filename: "custom-policy.md", + expectedGroup: "gh-aw-custom-workflow", + expectedCancel: boolPtr(true), + description: "Should use custom default policy", + }, + { + name: "pull request workflow with custom policy", + frontmatter: `--- +name: pr-custom-test +on: + pull_request: + types: [opened, synchronize] +concurrency_policy: + "*": + group: default-workflow + pull_requests: + group: pr-workflow + node: pull_request.number + cancel-in-progress: false +---`, + filename: "pr-custom.md", + expectedGroup: "gh-aw-pr-workflow-${{ github.event.pull_request.number }}", + expectedCancel: boolPtr(false), + description: "Should use PR-specific policy overriding cancel behavior", + }, + { + name: "issue workflow with node override", + frontmatter: `--- +name: issue-custom-test +on: + issues: + types: [opened] +concurrency_policy: + issues: + group: workflow + node: "github.event.issue.title" + cancel-in-progress: true +---`, + filename: "issue-custom.md", + expectedGroup: "gh-aw-${{ github.workflow }}-${{ github.event.issue.title }}", + expectedCancel: boolPtr(true), + description: "Should use custom node expression for issue workflow", + }, + { + name: "workflow with backwards compatible id field", + frontmatter: `--- +name: backwards-compat-test +on: + push: + branches: [main] +concurrency_policy: + "*": + id: legacy-workflow # using "id" instead of "group" +---`, + filename: "backwards-compat.md", + expectedGroup: "gh-aw-legacy-workflow", + expectedCancel: nil, + description: "Should support backwards compatible 'id' field", + }, + { + name: "schedule workflow with specific policy", + frontmatter: `--- +name: schedule-test +on: + schedule: + - cron: "0 9 * * *" +concurrency_policy: + schedule: + group: scheduled-tasks + cancel-in-progress: false +---`, + filename: "schedule.md", + expectedGroup: "gh-aw-scheduled-tasks", + expectedCancel: nil, + description: "Should use schedule-specific policy", + }, + { + name: "workflow with custom trigger policy", + frontmatter: `--- +name: custom-trigger-test +on: + repository_dispatch: + types: [custom-event] +concurrency_policy: + "*": + group: workflow + repository_dispatch: + group: dispatch-workflow + node: "github.event.client_payload.id" +---`, + filename: "custom-trigger.md", + expectedGroup: "gh-aw-dispatch-workflow-${{ github.event.client_payload.id }}", + expectedCancel: nil, + description: "Should use custom trigger-specific policy", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + testContent := tt.frontmatter + "\n\nThis is a test workflow for concurrency policy." + + testFile := filepath.Join(tmpDir, tt.filename) + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + // Parse the workflow to get its data + workflowData, err := compiler.parseWorkflowFile(testFile) + if err != nil { + t.Errorf("Failed to parse workflow: %v", err) + return + } + + t.Logf("Workflow: %s", tt.description) + t.Logf(" Expected Group: %s", tt.expectedGroup) + t.Logf(" Generated Concurrency: %s", workflowData.Concurrency) + + // Check that the concurrency field contains the expected group + if !strings.Contains(workflowData.Concurrency, tt.expectedGroup) { + t.Errorf("Expected concurrency to contain group '%s', got: %s", tt.expectedGroup, workflowData.Concurrency) + } + + // Check for cancel-in-progress behavior + hasCancel := strings.Contains(workflowData.Concurrency, "cancel-in-progress: true") + if tt.expectedCancel != nil { + if *tt.expectedCancel && !hasCancel { + t.Errorf("Expected cancel-in-progress: true, but not found in: %s", workflowData.Concurrency) + } else if !*tt.expectedCancel && hasCancel { + t.Errorf("Did not expect cancel-in-progress: true, but found in: %s", workflowData.Concurrency) + } + } + + // Ensure it's valid YAML format + if !strings.HasPrefix(workflowData.Concurrency, "concurrency:") { + t.Errorf("Generated concurrency should start with 'concurrency:', got: %s", workflowData.Concurrency) + } + }) + } +} + +func TestConcurrencyPolicyMerging(t *testing.T) { + // Test that user policies correctly override default policies + tmpDir, err := os.MkdirTemp("", "concurrency-policy-merging-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + compiler := NewCompiler(false, "", "test") + + // Test case: PR workflow with user policy that overrides the default PR behavior + frontmatter := `--- +name: pr-override-test +on: + pull_request: + types: [opened, synchronize] +concurrency_policy: + pull_requests: + group: custom-pr-group + node: "github.sha" + cancel-in-progress: false +---` + + testContent := frontmatter + "\n\nThis is a test for policy merging." + testFile := filepath.Join(tmpDir, "pr-override.md") + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + // Parse the workflow + workflowData, err := compiler.parseWorkflowFile(testFile) + if err != nil { + t.Errorf("Failed to parse workflow: %v", err) + return + } + + // Verify that the user policy overrode the defaults + expectedGroup := "gh-aw-custom-pr-group-${{ github.sha }}" + if !strings.Contains(workflowData.Concurrency, expectedGroup) { + t.Errorf("Expected custom group '%s', got: %s", expectedGroup, workflowData.Concurrency) + } + + // Verify that cancellation was overridden to false (should not appear) + if strings.Contains(workflowData.Concurrency, "cancel-in-progress: true") { + t.Errorf("Expected cancel-in-progress to be disabled, but found it enabled: %s", workflowData.Concurrency) + } +} + +func TestConcurrencyUserOverrideRespected(t *testing.T) { + // Test that user-provided concurrency is not overridden by the policy system + tmpDir, err := os.MkdirTemp("", "concurrency-user-override-test") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmpDir) + + compiler := NewCompiler(false, "", "test") + + frontmatter := `--- +name: user-override-test +concurrency: | + concurrency: + group: user-defined-group + cancel-in-progress: true +concurrency_policy: + "*": + group: policy-group +---` + + testContent := frontmatter + "\n\nThis is a test for user override." + testFile := filepath.Join(tmpDir, "user-override.md") + if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { + t.Fatal(err) + } + + // Parse the workflow + workflowData, err := compiler.parseWorkflowFile(testFile) + if err != nil { + t.Errorf("Failed to parse workflow: %v", err) + return + } + + // Should use the user-defined concurrency, not the policy + if !strings.Contains(workflowData.Concurrency, "user-defined-group") { + t.Errorf("Expected user-defined concurrency to be preserved, got: %s", workflowData.Concurrency) + } + + // Should not contain policy-generated content + if strings.Contains(workflowData.Concurrency, "policy-group") { + t.Errorf("User-defined concurrency should not be overridden by policy, got: %s", workflowData.Concurrency) + } +} diff --git a/pkg/workflow/concurrency_policy_test.go b/pkg/workflow/concurrency_policy_test.go new file mode 100644 index 00000000000..cdc70c0fcb4 --- /dev/null +++ b/pkg/workflow/concurrency_policy_test.go @@ -0,0 +1,499 @@ +package workflow + +import ( + "testing" +) + +func TestParseConcurrencyPolicyFromFrontmatter(t *testing.T) { + tests := []struct { + name string + frontmatter map[string]any + expected *ConcurrencyPolicySet + shouldError bool + description string + }{ + { + name: "empty frontmatter returns nil", + frontmatter: map[string]any{}, + expected: nil, + shouldError: false, + description: "No concurrency_policy key should return nil", + }, + { + name: "basic policy with default", + frontmatter: map[string]any{ + "concurrency_policy": map[string]any{ + "*": map[string]any{ + "group": "workflow", + }, + }, + }, + expected: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "workflow", + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + shouldError: false, + description: "Basic default policy should parse correctly", + }, + { + name: "complete policy set", + frontmatter: map[string]any{ + "concurrency_policy": map[string]any{ + "*": map[string]any{ + "group": "workflow", + }, + "issues": map[string]any{ + "group": "workflow", + "node": "issue.number", + "cancel-in-progress": true, + }, + "pull_requests": map[string]any{ + "group": "workflow", + "node": "pull_request.number", + "cancel-in-progress": true, + }, + }, + }, + expected: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "workflow", + }, + Issues: &ConcurrencyPolicy{ + Group: "workflow", + Node: "issue.number", + CancelInProgress: boolPtr(true), + }, + PullRequest: &ConcurrencyPolicy{ + Group: "workflow", + Node: "pull_request.number", + CancelInProgress: boolPtr(true), + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + shouldError: false, + description: "Complete policy set should parse correctly", + }, + { + name: "policy with backwards compatible id field", + frontmatter: map[string]any{ + "concurrency_policy": map[string]any{ + "*": map[string]any{ + "id": "workflow", // using "id" instead of "group" + }, + }, + }, + expected: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "workflow", + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + shouldError: false, + description: "Backwards compatible 'id' field should work", + }, + { + name: "invalid policy type", + frontmatter: map[string]any{ + "concurrency_policy": "invalid", + }, + expected: nil, + shouldError: true, + description: "Invalid policy type should return error", + }, + { + name: "invalid cancel-in-progress type", + frontmatter: map[string]any{ + "concurrency_policy": map[string]any{ + "*": map[string]any{ + "group": "workflow", + "cancel-in-progress": "invalid", + }, + }, + }, + expected: nil, + shouldError: true, + description: "Invalid cancel-in-progress type should return error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := parseConcurrencyPolicyFromFrontmatter(tt.frontmatter) + + if tt.shouldError { + if err == nil { + t.Errorf("Expected error but got none") + } + return + } + + if err != nil { + t.Errorf("Unexpected error: %v", err) + return + } + + if !comparePolicySets(result, tt.expected) { + t.Errorf("Policy sets don't match.\nGot: %+v\nExpected: %+v", result, tt.expected) + } + }) + } +} + +func TestComputeConcurrencyPolicy(t *testing.T) { + tests := []struct { + name string + workflowData *WorkflowData + isAliasTrigger bool + userPolicySet *ConcurrencyPolicySet + expected *ComputedConcurrencyPolicy + description string + }{ + { + name: "default policy for basic workflow", + workflowData: &WorkflowData{ + On: "push:", + }, + isAliasTrigger: false, + userPolicySet: nil, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}", + }, + description: "Basic workflow should use default policy", + }, + { + name: "pull request workflow with cancellation", + workflowData: &WorkflowData{ + On: "pull_request:\n types: [opened]", + }, + isAliasTrigger: false, + userPolicySet: nil, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-${{ github.ref }}", + CancelInProgress: boolPtr(true), + }, + description: "PR workflow should use PR policy with cancellation", + }, + { + name: "alias workflow without cancellation", + workflowData: &WorkflowData{ + On: "issues:\n types: [opened]", + }, + isAliasTrigger: true, + userPolicySet: nil, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}", + }, + description: "Alias workflow should not use cancellation", + }, + { + name: "user override policy", + workflowData: &WorkflowData{ + On: "push:", + }, + isAliasTrigger: false, + userPolicySet: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "custom-group", + CancelInProgress: boolPtr(true), + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-custom-group", + CancelInProgress: boolPtr(true), + }, + description: "User override should take precedence", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := computeConcurrencyPolicy(tt.workflowData, tt.isAliasTrigger, tt.userPolicySet) + + if err != nil { + t.Errorf("Unexpected error: %v", err) + return + } + + if !compareComputedPolicies(result, tt.expected) { + t.Errorf("Computed policies don't match.\nGot: %+v\nExpected: %+v", result, tt.expected) + } + }) + } +} + +func TestBuildGroupIdentifier(t *testing.T) { + tests := []struct { + name string + policy *ConcurrencyPolicy + workflowData *WorkflowData + expected string + description string + }{ + { + name: "basic workflow group", + policy: &ConcurrencyPolicy{ + Group: "workflow", + }, + workflowData: &WorkflowData{}, + expected: "gh-aw-${{ github.workflow }}", + description: "Basic workflow group should include workflow name", + }, + { + name: "issue number node", + policy: &ConcurrencyPolicy{ + Group: "workflow", + Node: "issue.number", + }, + workflowData: &WorkflowData{}, + expected: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}", + description: "Issue number node should be included", + }, + { + name: "pull request number node", + policy: &ConcurrencyPolicy{ + Group: "workflow", + Node: "pull_request.number", + }, + workflowData: &WorkflowData{}, + expected: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number }}", + description: "PR number node should be included", + }, + { + name: "custom group and node", + policy: &ConcurrencyPolicy{ + Group: "custom-group", + Node: "custom.expr", + }, + workflowData: &WorkflowData{}, + expected: "gh-aw-custom-group-${{ custom.expr }}", + description: "Custom group and node should be included", + }, + { + name: "custom node with expression", + policy: &ConcurrencyPolicy{ + Group: "workflow", + Node: "${{ github.event.custom.field }}", + }, + workflowData: &WorkflowData{}, + expected: "gh-aw-${{ github.workflow }}-${{ github.event.custom.field }}", + description: "Custom expression node should be preserved", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := buildGroupIdentifier(tt.policy, tt.workflowData) + + if result != tt.expected { + t.Errorf("Group identifier doesn't match.\nGot: %s\nExpected: %s", result, tt.expected) + } + }) + } +} + +func TestGenerateConcurrencyYAML(t *testing.T) { + tests := []struct { + name string + computed *ComputedConcurrencyPolicy + expected string + description string + }{ + { + name: "basic concurrency without cancel", + computed: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}", + }, + expected: "concurrency:\n group: \"gh-aw-${{ github.workflow }}\"", + description: "Basic concurrency should only include group", + }, + { + name: "concurrency with cancellation", + computed: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-${{ github.ref }}", + CancelInProgress: boolPtr(true), + }, + expected: "concurrency:\n group: \"gh-aw-${{ github.workflow }}-${{ github.ref }}\"\n cancel-in-progress: true", + description: "Concurrency with cancellation should include both fields", + }, + { + name: "concurrency with cancellation false", + computed: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}", + CancelInProgress: boolPtr(false), + }, + expected: "concurrency:\n group: \"gh-aw-${{ github.workflow }}\"", + description: "Concurrency with cancel false should not include cancel field", + }, + { + name: "nil computed policy", + computed: nil, + expected: "", + description: "Nil policy should return empty string", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := generateConcurrencyYAML(tt.computed) + + if result != tt.expected { + t.Errorf("YAML doesn't match.\nGot:\n%s\nExpected:\n%s", result, tt.expected) + } + }) + } +} + +func TestMergePolicySets(t *testing.T) { + tests := []struct { + name string + base *ConcurrencyPolicySet + override *ConcurrencyPolicySet + expected *ConcurrencyPolicySet + description string + }{ + { + name: "merge with nil override", + base: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{Group: "workflow"}, + Custom: make(map[string]*ConcurrencyPolicy), + }, + override: nil, + expected: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{Group: "workflow"}, + Custom: make(map[string]*ConcurrencyPolicy), + }, + description: "Merge with nil should return base", + }, + { + name: "override default policy", + base: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{Group: "workflow"}, + Custom: make(map[string]*ConcurrencyPolicy), + }, + override: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "custom-workflow", + CancelInProgress: boolPtr(true), + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + expected: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "custom-workflow", + CancelInProgress: boolPtr(true), + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + description: "Override should replace default policy", + }, + { + name: "partial override", + base: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "workflow", + Node: "base-node", + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + override: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + CancelInProgress: boolPtr(true), + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + expected: &ConcurrencyPolicySet{ + Default: &ConcurrencyPolicy{ + Group: "workflow", + Node: "base-node", + CancelInProgress: boolPtr(true), + }, + Custom: make(map[string]*ConcurrencyPolicy), + }, + description: "Partial override should merge fields", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := mergePolicySets(tt.base, tt.override) + + if !comparePolicySets(result, tt.expected) { + t.Errorf("Merged policy sets don't match.\nGot: %+v\nExpected: %+v", result, tt.expected) + } + }) + } +} + +// Helper functions for testing + +func boolPtr(b bool) *bool { + return &b +} + +func comparePolicySets(a, b *ConcurrencyPolicySet) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + + return comparePolicies(a.Default, b.Default) && + comparePolicies(a.Issues, b.Issues) && + comparePolicies(a.PullRequest, b.PullRequest) && + comparePolicies(a.Schedule, b.Schedule) && + comparePolicies(a.Manual, b.Manual) && + compareCustomPolicies(a.Custom, b.Custom) +} + +func comparePolicies(a, b *ConcurrencyPolicy) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + + return a.Group == b.Group && + a.Node == b.Node && + compareBoolPtrs(a.CancelInProgress, b.CancelInProgress) +} + +func compareBoolPtrs(a, b *bool) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + return *a == *b +} + +func compareCustomPolicies(a, b map[string]*ConcurrencyPolicy) bool { + if len(a) != len(b) { + return false + } + + for k, v := range a { + if !comparePolicies(v, b[k]) { + return false + } + } + + return true +} + +func compareComputedPolicies(a, b *ComputedConcurrencyPolicy) bool { + if a == nil && b == nil { + return true + } + if a == nil || b == nil { + return false + } + + return a.Group == b.Group && + compareBoolPtrs(a.CancelInProgress, b.CancelInProgress) +} From 820cd8b82b4aa47a85f856d8e7b86f5c530dded8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 Aug 2025 11:22:47 +0000 Subject: [PATCH 3/4] Remove concurrency_policy from schema, maintain internal processing only Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/schema.go | 28 ++++++++++++++++++-- pkg/parser/schemas/main_workflow_schema.json | 27 ------------------- 2 files changed, 26 insertions(+), 29 deletions(-) diff --git a/pkg/parser/schema.go b/pkg/parser/schema.go index b7228d17466..d40284c7783 100644 --- a/pkg/parser/schema.go +++ b/pkg/parser/schema.go @@ -23,12 +23,16 @@ var mcpConfigSchema string // ValidateMainWorkflowFrontmatterWithSchema validates main workflow frontmatter using JSON schema func ValidateMainWorkflowFrontmatterWithSchema(frontmatter map[string]any) error { - return validateWithSchema(frontmatter, mainWorkflowSchema, "main workflow file") + // Remove internal-only fields from validation + cleanedFrontmatter := removeInternalFields(frontmatter) + return validateWithSchema(cleanedFrontmatter, mainWorkflowSchema, "main workflow file") } // ValidateMainWorkflowFrontmatterWithSchemaAndLocation validates main workflow frontmatter with file location info func ValidateMainWorkflowFrontmatterWithSchemaAndLocation(frontmatter map[string]any, filePath string) error { - return validateWithSchemaAndLocation(frontmatter, mainWorkflowSchema, "main workflow file", filePath) + // Remove internal-only fields from validation + cleanedFrontmatter := removeInternalFields(frontmatter) + return validateWithSchemaAndLocation(cleanedFrontmatter, mainWorkflowSchema, "main workflow file", filePath) } // ValidateIncludedFileFrontmatterWithSchema validates included file frontmatter using JSON schema @@ -203,3 +207,23 @@ func min(a, b int) int { } return b } + +// removeInternalFields creates a copy of the frontmatter without internal-only fields +// that should not be validated against the public schema +func removeInternalFields(frontmatter map[string]any) map[string]any { + if frontmatter == nil { + return nil + } + + // Create a copy of the frontmatter + cleaned := make(map[string]any) + for key, value := range frontmatter { + // Skip internal-only fields that are processed by Go code but not part of the public schema + if key == "concurrency_policy" { + continue + } + cleaned[key] = value + } + + return cleaned +} diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index af51ac9ea42..809fa8d61e0 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -223,33 +223,6 @@ } ] }, - "concurrency_policy": { - "type": "object", - "description": "Concurrency policy configuration for different workflow trigger types", - "additionalProperties": { - "type": "object", - "description": "Policy configuration for a specific trigger type", - "properties": { - "group": { - "type": "string", - "description": "Group identifier for concurrency (defaults to 'workflow')" - }, - "id": { - "type": "string", - "description": "Legacy group identifier (use 'group' instead)" - }, - "node": { - "type": "string", - "description": "Node expression for dynamic concurrency (e.g., 'issue.number', 'pull_request.number', 'github.ref')" - }, - "cancel-in-progress": { - "type": "boolean", - "description": "Whether to cancel in-progress workflows when new ones start" - } - }, - "additionalProperties": false - } - }, "env": { "description": "Environment variables for the workflow", "oneOf": [ From bd3166da393b23454360fe5ef4c947b7ea295ac7 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 19 Aug 2025 13:09:25 +0000 Subject: [PATCH 4/4] Remove concurrency_policy frontmatter support, keep code-based rules only Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/parser/schema.go | 6 +- pkg/workflow/concurrency.go | 41 +- pkg/workflow/concurrency_policy.go | 200 +------ .../concurrency_policy_integration_test.go | 168 ++---- pkg/workflow/concurrency_policy_test.go | 537 ++++++------------ 5 files changed, 224 insertions(+), 728 deletions(-) diff --git a/pkg/parser/schema.go b/pkg/parser/schema.go index d40284c7783..5e5f8323b3a 100644 --- a/pkg/parser/schema.go +++ b/pkg/parser/schema.go @@ -218,10 +218,8 @@ func removeInternalFields(frontmatter map[string]any) map[string]any { // Create a copy of the frontmatter cleaned := make(map[string]any) for key, value := range frontmatter { - // Skip internal-only fields that are processed by Go code but not part of the public schema - if key == "concurrency_policy" { - continue - } + // Currently no internal-only fields need to be filtered + // This function is kept for future extensibility cleaned[key] = value } diff --git a/pkg/workflow/concurrency.go b/pkg/workflow/concurrency.go index 55ec4f4e1b8..d10e86dd4e0 100644 --- a/pkg/workflow/concurrency.go +++ b/pkg/workflow/concurrency.go @@ -1,10 +1,7 @@ package workflow import ( - "fmt" "strings" - - "github.com/githubnext/gh-aw/pkg/console" ) // GenerateConcurrencyConfig generates the concurrency configuration for a workflow @@ -20,47 +17,21 @@ func GenerateConcurrencyConfig(workflowData *WorkflowData, isAliasTrigger bool) } // GenerateConcurrencyConfigWithFrontmatter generates concurrency config using the policy system -// This is the new entry point that accepts frontmatter for policy parsing +// This function maintains the same interface but no longer parses frontmatter for policies func GenerateConcurrencyConfigWithFrontmatter(workflowData *WorkflowData, isAliasTrigger bool, frontmatter map[string]any, verbose bool) string { // Don't override if already set by user if workflowData.Concurrency != "" { return workflowData.Concurrency } - // Parse concurrency policy from frontmatter - userPolicySet, err := parseConcurrencyPolicyFromFrontmatter(frontmatter) - if err != nil { - if verbose { - fmt.Println(console.FormatWarningMessage("Failed to parse concurrency policy, using defaults: " + err.Error())) - } - // Fall back to legacy behavior - return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) - } - - // Compute the final policy - computed, err := computeConcurrencyPolicy(workflowData, isAliasTrigger, userPolicySet) - if err != nil { - if verbose { - fmt.Println(console.FormatWarningMessage("Failed to compute concurrency policy, using defaults: " + err.Error())) - } - // Fall back to legacy behavior - return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) - } - - // Generate YAML - yaml := generateConcurrencyYAML(computed) - if yaml == "" { - // Fall back to legacy behavior - return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) - } - - return yaml + // Use the policy system with code-based rules only + return generateConcurrencyWithPolicySystem(workflowData, isAliasTrigger) } -// generateConcurrencyWithPolicySystem uses the new policy system but with default policies only +// generateConcurrencyWithPolicySystem uses the policy system with code-based rules only func generateConcurrencyWithPolicySystem(workflowData *WorkflowData, isAliasTrigger bool) string { - // Compute policy using defaults only (no user override) - computed, err := computeConcurrencyPolicy(workflowData, isAliasTrigger, nil) + // Compute policy using code-based rules only + computed, err := computeConcurrencyPolicy(workflowData, isAliasTrigger) if err != nil { // Fall back to legacy behavior if policy system fails return generateLegacyConcurrency(workflowData, isAliasTrigger) diff --git a/pkg/workflow/concurrency_policy.go b/pkg/workflow/concurrency_policy.go index b8ab9e83d6f..3b19837ed18 100644 --- a/pkg/workflow/concurrency_policy.go +++ b/pkg/workflow/concurrency_policy.go @@ -28,106 +28,13 @@ type ComputedConcurrencyPolicy struct { CancelInProgress *bool } -// parseConcurrencyPolicyFromFrontmatter extracts concurrency policy configuration from frontmatter -func parseConcurrencyPolicyFromFrontmatter(frontmatter map[string]any) (*ConcurrencyPolicySet, error) { - // Look for "concurrency_policy" key in frontmatter - policyValue, exists := frontmatter["concurrency_policy"] - if !exists { - return nil, nil // No policy defined, use defaults - } - - policyMap, ok := policyValue.(map[string]any) - if !ok { - return nil, fmt.Errorf("concurrency_policy must be an object") - } - - policySet := &ConcurrencyPolicySet{ - Custom: make(map[string]*ConcurrencyPolicy), - } - - // Parse each policy context - for key, value := range policyMap { - valueMap, ok := value.(map[string]any) - if !ok { - return nil, fmt.Errorf("concurrency_policy.%s must be an object", key) - } - - policy, err := parseSinglePolicy(valueMap) - if err != nil { - return nil, fmt.Errorf("invalid concurrency_policy.%s: %w", key, err) - } - - // Assign to the appropriate field based on key - switch key { - case "*": - policySet.Default = policy - case "issues": - policySet.Issues = policy - case "pull_requests": - policySet.PullRequest = policy - case "schedule": - policySet.Schedule = policy - case "workflow_dispatch": - policySet.Manual = policy - default: - policySet.Custom[key] = policy - } - } - - return policySet, nil -} - -// parseSinglePolicy parses a single policy definition -func parseSinglePolicy(policyMap map[string]any) (*ConcurrencyPolicy, error) { - policy := &ConcurrencyPolicy{} - - // Parse group (can be either "group" or "id" for backwards compatibility) - if group, exists := policyMap["group"]; exists { - if groupStr, ok := group.(string); ok { - policy.Group = groupStr - } else { - return nil, fmt.Errorf("group must be a string") - } - } else if id, exists := policyMap["id"]; exists { - // Support "id" for backwards compatibility (mentioned in issue) - if idStr, ok := id.(string); ok { - policy.Group = idStr - } else { - return nil, fmt.Errorf("id must be a string") - } - } - - // Parse node - if node, exists := policyMap["node"]; exists { - if nodeStr, ok := node.(string); ok { - policy.Node = nodeStr - } else { - return nil, fmt.Errorf("node must be a string") - } - } - - // Parse cancel-in-progress - if cancel, exists := policyMap["cancel-in-progress"]; exists { - if cancelBool, ok := cancel.(bool); ok { - policy.CancelInProgress = &cancelBool - } else { - return nil, fmt.Errorf("cancel-in-progress must be a boolean") - } - } - - return policy, nil -} - -// computeConcurrencyPolicy merges policies from different sources and computes the final configuration -func computeConcurrencyPolicy(workflowData *WorkflowData, isAliasTrigger bool, userPolicySet *ConcurrencyPolicySet) (*ComputedConcurrencyPolicy, error) { - // Start with default policies based on workflow characteristics - defaultPolicySet := getDefaultPolicySet(isAliasTrigger) - - // Merge user-defined policies if provided - finalPolicySet := mergePolicySets(defaultPolicySet, userPolicySet) +// computeConcurrencyPolicy computes the final concurrency configuration based on workflow characteristics +func computeConcurrencyPolicy(workflowData *WorkflowData, isAliasTrigger bool) (*ComputedConcurrencyPolicy, error) { + // Get default policies based on workflow characteristics + policySet := getDefaultPolicySet(isAliasTrigger) // Determine which specific policy to use based on workflow triggers - selectedPolicy := selectPolicyForWorkflow(workflowData, isAliasTrigger, finalPolicySet) + selectedPolicy := selectPolicyForWorkflow(workflowData, isAliasTrigger, policySet) // Compute the final concurrency configuration computed := &ComputedConcurrencyPolicy{} @@ -180,103 +87,6 @@ func getDefaultPolicySet(isAliasTrigger bool) *ConcurrencyPolicySet { return policySet } -// mergePolicySets merges user-defined policies with default policies -func mergePolicySets(base, override *ConcurrencyPolicySet) *ConcurrencyPolicySet { - if override == nil { - return base - } - - result := &ConcurrencyPolicySet{ - Custom: make(map[string]*ConcurrencyPolicy), - } - - // Copy base policies - if base.Default != nil { - result.Default = copyPolicy(base.Default) - } - if base.Issues != nil { - result.Issues = copyPolicy(base.Issues) - } - if base.PullRequest != nil { - result.PullRequest = copyPolicy(base.PullRequest) - } - if base.Schedule != nil { - result.Schedule = copyPolicy(base.Schedule) - } - if base.Manual != nil { - result.Manual = copyPolicy(base.Manual) - } - for k, v := range base.Custom { - result.Custom[k] = copyPolicy(v) - } - - // Override with user-defined policies - if override.Default != nil { - result.Default = mergePolicy(result.Default, override.Default) - } - if override.Issues != nil { - result.Issues = mergePolicy(result.Issues, override.Issues) - } - if override.PullRequest != nil { - result.PullRequest = mergePolicy(result.PullRequest, override.PullRequest) - } - if override.Schedule != nil { - result.Schedule = mergePolicy(result.Schedule, override.Schedule) - } - if override.Manual != nil { - result.Manual = mergePolicy(result.Manual, override.Manual) - } - for k, v := range override.Custom { - result.Custom[k] = mergePolicy(result.Custom[k], v) - } - - return result -} - -// mergePolicy merges two policies, with override taking precedence -func mergePolicy(base, override *ConcurrencyPolicy) *ConcurrencyPolicy { - if override == nil { - return base - } - if base == nil { - return copyPolicy(override) - } - - result := copyPolicy(base) - - // Override fields if specified - if override.Group != "" { - result.Group = override.Group - } - if override.Node != "" { - result.Node = override.Node - } - if override.CancelInProgress != nil { - result.CancelInProgress = override.CancelInProgress - } - - return result -} - -// copyPolicy creates a deep copy of a policy -func copyPolicy(policy *ConcurrencyPolicy) *ConcurrencyPolicy { - if policy == nil { - return nil - } - - result := &ConcurrencyPolicy{ - Group: policy.Group, - Node: policy.Node, - } - - if policy.CancelInProgress != nil { - cancel := *policy.CancelInProgress - result.CancelInProgress = &cancel - } - - return result -} - // selectPolicyForWorkflow selects the most appropriate policy for the given workflow func selectPolicyForWorkflow(workflowData *WorkflowData, isAliasTrigger bool, policySet *ConcurrencyPolicySet) *ConcurrencyPolicy { if isAliasTrigger { diff --git a/pkg/workflow/concurrency_policy_integration_test.go b/pkg/workflow/concurrency_policy_integration_test.go index 70799d6d179..0e9b6af646c 100644 --- a/pkg/workflow/concurrency_policy_integration_test.go +++ b/pkg/workflow/concurrency_policy_integration_test.go @@ -8,7 +8,7 @@ import ( ) func TestConcurrencyPolicyIntegration(t *testing.T) { - // Test the new concurrency policy system with real workflow files + // Test the concurrency policy system with real workflow files using code-based rules tmpDir, err := os.MkdirTemp("", "concurrency-policy-integration-test") if err != nil { t.Fatal(err) @@ -26,115 +26,68 @@ func TestConcurrencyPolicyIntegration(t *testing.T) { description string }{ { - name: "workflow with custom concurrency policy", + name: "basic push workflow", frontmatter: `--- -name: custom-concurrency-test +name: basic-push-test on: push: branches: [main] -concurrency_policy: - "*": - group: custom-workflow - cancel-in-progress: true - pull_requests: - group: pr-specific - node: pull_request.number - cancel-in-progress: false ---`, - filename: "custom-policy.md", - expectedGroup: "gh-aw-custom-workflow", - expectedCancel: boolPtr(true), - description: "Should use custom default policy", + filename: "basic-push.md", + expectedGroup: "gh-aw-${{ github.workflow }}", + expectedCancel: nil, + description: "Should use basic group for push workflows", }, { - name: "pull request workflow with custom policy", + name: "pull request workflow", frontmatter: `--- -name: pr-custom-test +name: pr-test on: pull_request: types: [opened, synchronize] -concurrency_policy: - "*": - group: default-workflow - pull_requests: - group: pr-workflow - node: pull_request.number - cancel-in-progress: false ---`, - filename: "pr-custom.md", - expectedGroup: "gh-aw-pr-workflow-${{ github.event.pull_request.number }}", - expectedCancel: boolPtr(false), - description: "Should use PR-specific policy overriding cancel behavior", + filename: "pr.md", + expectedGroup: "gh-aw-${{ github.workflow }}-${{ github.ref }}", + expectedCancel: boolPtr(true), + description: "Should use ref-based group with cancellation for PR workflows", }, { - name: "issue workflow with node override", + name: "issue workflow", frontmatter: `--- -name: issue-custom-test +name: issue-test on: issues: types: [opened] -concurrency_policy: - issues: - group: workflow - node: "github.event.issue.title" - cancel-in-progress: true ---`, - filename: "issue-custom.md", - expectedGroup: "gh-aw-${{ github.workflow }}-${{ github.event.issue.title }}", + filename: "issue.md", + expectedGroup: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}", expectedCancel: boolPtr(true), - description: "Should use custom node expression for issue workflow", + description: "Should use issue number with cancellation for issue workflows", }, { - name: "workflow with backwards compatible id field", - frontmatter: `--- -name: backwards-compat-test -on: - push: - branches: [main] -concurrency_policy: - "*": - id: legacy-workflow # using "id" instead of "group" ----`, - filename: "backwards-compat.md", - expectedGroup: "gh-aw-legacy-workflow", - expectedCancel: nil, - description: "Should support backwards compatible 'id' field", - }, - { - name: "schedule workflow with specific policy", + name: "schedule workflow", frontmatter: `--- name: schedule-test on: schedule: - cron: "0 9 * * *" -concurrency_policy: - schedule: - group: scheduled-tasks - cancel-in-progress: false ---`, filename: "schedule.md", - expectedGroup: "gh-aw-scheduled-tasks", + expectedGroup: "gh-aw-${{ github.workflow }}", expectedCancel: nil, - description: "Should use schedule-specific policy", + description: "Should use basic group for scheduled workflows", }, { - name: "workflow with custom trigger policy", + name: "workflow_dispatch workflow", frontmatter: `--- -name: custom-trigger-test +name: manual-test on: - repository_dispatch: - types: [custom-event] -concurrency_policy: - "*": - group: workflow - repository_dispatch: - group: dispatch-workflow - node: "github.event.client_payload.id" + workflow_dispatch: ---`, - filename: "custom-trigger.md", - expectedGroup: "gh-aw-dispatch-workflow-${{ github.event.client_payload.id }}", + filename: "manual.md", + expectedGroup: "gh-aw-${{ github.workflow }}", expectedCancel: nil, - description: "Should use custom trigger-specific policy", + description: "Should use basic group for manual workflows", }, } @@ -181,54 +134,6 @@ concurrency_policy: } } -func TestConcurrencyPolicyMerging(t *testing.T) { - // Test that user policies correctly override default policies - tmpDir, err := os.MkdirTemp("", "concurrency-policy-merging-test") - if err != nil { - t.Fatal(err) - } - defer os.RemoveAll(tmpDir) - - compiler := NewCompiler(false, "", "test") - - // Test case: PR workflow with user policy that overrides the default PR behavior - frontmatter := `--- -name: pr-override-test -on: - pull_request: - types: [opened, synchronize] -concurrency_policy: - pull_requests: - group: custom-pr-group - node: "github.sha" - cancel-in-progress: false ----` - - testContent := frontmatter + "\n\nThis is a test for policy merging." - testFile := filepath.Join(tmpDir, "pr-override.md") - if err := os.WriteFile(testFile, []byte(testContent), 0644); err != nil { - t.Fatal(err) - } - - // Parse the workflow - workflowData, err := compiler.parseWorkflowFile(testFile) - if err != nil { - t.Errorf("Failed to parse workflow: %v", err) - return - } - - // Verify that the user policy overrode the defaults - expectedGroup := "gh-aw-custom-pr-group-${{ github.sha }}" - if !strings.Contains(workflowData.Concurrency, expectedGroup) { - t.Errorf("Expected custom group '%s', got: %s", expectedGroup, workflowData.Concurrency) - } - - // Verify that cancellation was overridden to false (should not appear) - if strings.Contains(workflowData.Concurrency, "cancel-in-progress: true") { - t.Errorf("Expected cancel-in-progress to be disabled, but found it enabled: %s", workflowData.Concurrency) - } -} - func TestConcurrencyUserOverrideRespected(t *testing.T) { // Test that user-provided concurrency is not overridden by the policy system tmpDir, err := os.MkdirTemp("", "concurrency-user-override-test") @@ -241,13 +146,13 @@ func TestConcurrencyUserOverrideRespected(t *testing.T) { frontmatter := `--- name: user-override-test +on: + push: + branches: [main] concurrency: | concurrency: group: user-defined-group cancel-in-progress: true -concurrency_policy: - "*": - group: policy-group ---` testContent := frontmatter + "\n\nThis is a test for user override." @@ -263,13 +168,18 @@ concurrency_policy: return } - // Should use the user-defined concurrency, not the policy + // Should use the user-defined concurrency, not the auto-generated policy if !strings.Contains(workflowData.Concurrency, "user-defined-group") { t.Errorf("Expected user-defined concurrency to be preserved, got: %s", workflowData.Concurrency) } - // Should not contain policy-generated content - if strings.Contains(workflowData.Concurrency, "policy-group") { - t.Errorf("User-defined concurrency should not be overridden by policy, got: %s", workflowData.Concurrency) + // Should not contain auto-generated content + if strings.Contains(workflowData.Concurrency, "gh-aw-${{ github.workflow }}") { + t.Errorf("User-defined concurrency should not be overridden by auto-generated policy, got: %s", workflowData.Concurrency) } } + +// Helper function to create bool pointers +func boolPtr(b bool) *bool { + return &b +} diff --git a/pkg/workflow/concurrency_policy_test.go b/pkg/workflow/concurrency_policy_test.go index cdc70c0fcb4..9a51852bc1e 100644 --- a/pkg/workflow/concurrency_policy_test.go +++ b/pkg/workflow/concurrency_policy_test.go @@ -4,221 +4,213 @@ import ( "testing" ) -func TestParseConcurrencyPolicyFromFrontmatter(t *testing.T) { +func TestComputeConcurrencyPolicy(t *testing.T) { tests := []struct { - name string - frontmatter map[string]any - expected *ConcurrencyPolicySet - shouldError bool - description string + name string + workflowData *WorkflowData + isAliasTrigger bool + expected *ComputedConcurrencyPolicy + description string }{ { - name: "empty frontmatter returns nil", - frontmatter: map[string]any{}, - expected: nil, - shouldError: false, - description: "No concurrency_policy key should return nil", + name: "basic workflow without special triggers", + workflowData: &WorkflowData{ + On: "push:", + }, + isAliasTrigger: false, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}", + CancelInProgress: nil, + }, + description: "Basic workflow should use simple group", }, { - name: "basic policy with default", - frontmatter: map[string]any{ - "concurrency_policy": map[string]any{ - "*": map[string]any{ - "group": "workflow", - }, - }, + name: "pull request workflow", + workflowData: &WorkflowData{ + On: "pull_request:", }, - expected: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "workflow", - }, - Custom: make(map[string]*ConcurrencyPolicy), + isAliasTrigger: false, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-${{ github.ref }}", + CancelInProgress: &[]bool{true}[0], }, - shouldError: false, - description: "Basic default policy should parse correctly", + description: "Pull request workflow should include ref and enable cancellation", }, { - name: "complete policy set", - frontmatter: map[string]any{ - "concurrency_policy": map[string]any{ - "*": map[string]any{ - "group": "workflow", - }, - "issues": map[string]any{ - "group": "workflow", - "node": "issue.number", - "cancel-in-progress": true, - }, - "pull_requests": map[string]any{ - "group": "workflow", - "node": "pull_request.number", - "cancel-in-progress": true, - }, - }, + name: "issues workflow", + workflowData: &WorkflowData{ + On: "issues:", }, - expected: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "workflow", - }, - Issues: &ConcurrencyPolicy{ - Group: "workflow", - Node: "issue.number", - CancelInProgress: boolPtr(true), - }, - PullRequest: &ConcurrencyPolicy{ - Group: "workflow", - Node: "pull_request.number", - CancelInProgress: boolPtr(true), - }, - Custom: make(map[string]*ConcurrencyPolicy), + isAliasTrigger: false, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}", + CancelInProgress: &[]bool{true}[0], }, - shouldError: false, - description: "Complete policy set should parse correctly", + description: "Issues workflow should include issue number and enable cancellation", }, { - name: "policy with backwards compatible id field", - frontmatter: map[string]any{ - "concurrency_policy": map[string]any{ - "*": map[string]any{ - "id": "workflow", // using "id" instead of "group" - }, - }, + name: "alias workflow", + workflowData: &WorkflowData{ + On: "issues:", }, - expected: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "workflow", - }, - Custom: make(map[string]*ConcurrencyPolicy), + isAliasTrigger: true, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}", + CancelInProgress: nil, }, - shouldError: false, - description: "Backwards compatible 'id' field should work", + description: "Alias workflow should not enable cancellation", }, { - name: "invalid policy type", - frontmatter: map[string]any{ - "concurrency_policy": "invalid", + name: "schedule workflow", + workflowData: &WorkflowData{ + On: "schedule:", + }, + isAliasTrigger: false, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}", + CancelInProgress: nil, }, - expected: nil, - shouldError: true, - description: "Invalid policy type should return error", + description: "Schedule workflow should use default policy", }, { - name: "invalid cancel-in-progress type", - frontmatter: map[string]any{ - "concurrency_policy": map[string]any{ - "*": map[string]any{ - "group": "workflow", - "cancel-in-progress": "invalid", - }, - }, + name: "workflow_dispatch workflow", + workflowData: &WorkflowData{ + On: "workflow_dispatch:", + }, + isAliasTrigger: false, + expected: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}", + CancelInProgress: nil, }, - expected: nil, - shouldError: true, - description: "Invalid cancel-in-progress type should return error", + description: "Manual workflow should use default policy", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := parseConcurrencyPolicyFromFrontmatter(tt.frontmatter) - - if tt.shouldError { - if err == nil { - t.Errorf("Expected error but got none") - } - return - } + result, err := computeConcurrencyPolicy(tt.workflowData, tt.isAliasTrigger) if err != nil { t.Errorf("Unexpected error: %v", err) return } - if !comparePolicySets(result, tt.expected) { - t.Errorf("Policy sets don't match.\nGot: %+v\nExpected: %+v", result, tt.expected) + if result.Group != tt.expected.Group { + t.Errorf("Group mismatch.\nGot: %s\nExpected: %s", result.Group, tt.expected.Group) + } + + if !compareCancelInProgress(result.CancelInProgress, tt.expected.CancelInProgress) { + t.Errorf("CancelInProgress mismatch.\nGot: %v\nExpected: %v", result.CancelInProgress, tt.expected.CancelInProgress) } }) } } -func TestComputeConcurrencyPolicy(t *testing.T) { +func TestGenerateConcurrencyYAML(t *testing.T) { tests := []struct { - name string - workflowData *WorkflowData - isAliasTrigger bool - userPolicySet *ConcurrencyPolicySet - expected *ComputedConcurrencyPolicy - description string + name string + computed *ComputedConcurrencyPolicy + expected string }{ { - name: "default policy for basic workflow", - workflowData: &WorkflowData{ - On: "push:", - }, - isAliasTrigger: false, - userPolicySet: nil, - expected: &ComputedConcurrencyPolicy{ + name: "basic group without cancellation", + computed: &ComputedConcurrencyPolicy{ Group: "gh-aw-${{ github.workflow }}", }, - description: "Basic workflow should use default policy", + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}"`, }, { - name: "pull request workflow with cancellation", - workflowData: &WorkflowData{ - On: "pull_request:\n types: [opened]", - }, - isAliasTrigger: false, - userPolicySet: nil, - expected: &ComputedConcurrencyPolicy{ - Group: "gh-aw-${{ github.workflow }}-${{ github.ref }}", - CancelInProgress: boolPtr(true), + name: "group with cancellation enabled", + computed: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-ref", + CancelInProgress: &[]bool{true}[0], }, - description: "PR workflow should use PR policy with cancellation", + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}-ref" + cancel-in-progress: true`, }, { - name: "alias workflow without cancellation", - workflowData: &WorkflowData{ - On: "issues:\n types: [opened]", - }, - isAliasTrigger: true, - userPolicySet: nil, - expected: &ComputedConcurrencyPolicy{ - Group: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number || github.event.pull_request.number }}", + name: "group with cancellation disabled", + computed: &ComputedConcurrencyPolicy{ + Group: "gh-aw-${{ github.workflow }}-ref", + CancelInProgress: &[]bool{false}[0], }, - description: "Alias workflow should not use cancellation", + expected: `concurrency: + group: "gh-aw-${{ github.workflow }}-ref"`, }, { - name: "user override policy", - workflowData: &WorkflowData{ - On: "push:", - }, + name: "nil policy returns empty string", + computed: nil, + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := generateConcurrencyYAML(tt.computed) + if result != tt.expected { + t.Errorf("YAML output mismatch.\nGot:\n%s\nExpected:\n%s", result, tt.expected) + } + }) + } +} + +func TestGetDefaultPolicySet(t *testing.T) { + tests := []struct { + name string + isAliasTrigger bool + description string + }{ + { + name: "normal workflow policy set", isAliasTrigger: false, - userPolicySet: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "custom-group", - CancelInProgress: boolPtr(true), - }, - Custom: make(map[string]*ConcurrencyPolicy), - }, - expected: &ComputedConcurrencyPolicy{ - Group: "gh-aw-custom-group", - CancelInProgress: boolPtr(true), - }, - description: "User override should take precedence", + description: "Normal workflows should have cancellation enabled for issues and PR triggers", + }, + { + name: "alias workflow policy set", + isAliasTrigger: true, + description: "Alias workflows should not have cancellation enabled", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result, err := computeConcurrencyPolicy(tt.workflowData, tt.isAliasTrigger, tt.userPolicySet) + policySet := getDefaultPolicySet(tt.isAliasTrigger) - if err != nil { - t.Errorf("Unexpected error: %v", err) + // Verify basic structure + if policySet == nil { + t.Error("Policy set should not be nil") return } - if !compareComputedPolicies(result, tt.expected) { - t.Errorf("Computed policies don't match.\nGot: %+v\nExpected: %+v", result, tt.expected) + if policySet.Default == nil { + t.Error("Default policy should not be nil") + } + + if policySet.Issues == nil { + t.Error("Issues policy should not be nil") + } + + if policySet.PullRequest == nil { + t.Error("PullRequest policy should not be nil") + } + + // Verify cancellation behavior based on alias trigger + if tt.isAliasTrigger { + if policySet.Issues.CancelInProgress != nil { + t.Error("Alias workflow issues policy should not have cancellation enabled") + } + if policySet.PullRequest.CancelInProgress != nil { + t.Error("Alias workflow PR policy should not have cancellation enabled") + } + } else { + if policySet.Issues.CancelInProgress == nil || !*policySet.Issues.CancelInProgress { + t.Error("Normal workflow issues policy should have cancellation enabled") + } + if policySet.PullRequest.CancelInProgress == nil || !*policySet.PullRequest.CancelInProgress { + t.Error("Normal workflow PR policy should have cancellation enabled") + } } }) } @@ -230,7 +222,6 @@ func TestBuildGroupIdentifier(t *testing.T) { policy *ConcurrencyPolicy workflowData *WorkflowData expected string - description string }{ { name: "basic workflow group", @@ -239,230 +230,72 @@ func TestBuildGroupIdentifier(t *testing.T) { }, workflowData: &WorkflowData{}, expected: "gh-aw-${{ github.workflow }}", - description: "Basic workflow group should include workflow name", }, { - name: "issue number node", + name: "custom group", + policy: &ConcurrencyPolicy{ + Group: "custom", + }, + workflowData: &WorkflowData{}, + expected: "gh-aw-custom", + }, + { + name: "with issue number node", policy: &ConcurrencyPolicy{ Group: "workflow", Node: "issue.number", }, workflowData: &WorkflowData{}, expected: "gh-aw-${{ github.workflow }}-${{ github.event.issue.number }}", - description: "Issue number node should be included", }, { - name: "pull request number node", + name: "with pull request number node", policy: &ConcurrencyPolicy{ Group: "workflow", Node: "pull_request.number", }, workflowData: &WorkflowData{}, expected: "gh-aw-${{ github.workflow }}-${{ github.event.pull_request.number }}", - description: "PR number node should be included", }, { - name: "custom group and node", + name: "with github ref node", policy: &ConcurrencyPolicy{ - Group: "custom-group", - Node: "custom.expr", + Group: "workflow", + Node: "github.ref", }, workflowData: &WorkflowData{}, - expected: "gh-aw-custom-group-${{ custom.expr }}", - description: "Custom group and node should be included", + expected: "gh-aw-${{ github.workflow }}-${{ github.ref }}", }, { - name: "custom node with expression", + name: "with custom node expression", policy: &ConcurrencyPolicy{ Group: "workflow", - Node: "${{ github.event.custom.field }}", + Node: "custom.expression", }, workflowData: &WorkflowData{}, - expected: "gh-aw-${{ github.workflow }}-${{ github.event.custom.field }}", - description: "Custom expression node should be preserved", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := buildGroupIdentifier(tt.policy, tt.workflowData) - - if result != tt.expected { - t.Errorf("Group identifier doesn't match.\nGot: %s\nExpected: %s", result, tt.expected) - } - }) - } -} - -func TestGenerateConcurrencyYAML(t *testing.T) { - tests := []struct { - name string - computed *ComputedConcurrencyPolicy - expected string - description string - }{ - { - name: "basic concurrency without cancel", - computed: &ComputedConcurrencyPolicy{ - Group: "gh-aw-${{ github.workflow }}", - }, - expected: "concurrency:\n group: \"gh-aw-${{ github.workflow }}\"", - description: "Basic concurrency should only include group", + expected: "gh-aw-${{ github.workflow }}-${{ custom.expression }}", }, { - name: "concurrency with cancellation", - computed: &ComputedConcurrencyPolicy{ - Group: "gh-aw-${{ github.workflow }}-${{ github.ref }}", - CancelInProgress: boolPtr(true), - }, - expected: "concurrency:\n group: \"gh-aw-${{ github.workflow }}-${{ github.ref }}\"\n cancel-in-progress: true", - description: "Concurrency with cancellation should include both fields", - }, - { - name: "concurrency with cancellation false", - computed: &ComputedConcurrencyPolicy{ - Group: "gh-aw-${{ github.workflow }}", - CancelInProgress: boolPtr(false), - }, - expected: "concurrency:\n group: \"gh-aw-${{ github.workflow }}\"", - description: "Concurrency with cancel false should not include cancel field", - }, - { - name: "nil computed policy", - computed: nil, - expected: "", - description: "Nil policy should return empty string", + name: "nil policy returns fallback", + policy: nil, + workflowData: &WorkflowData{}, + expected: "gh-aw-${{ github.workflow }}", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := generateConcurrencyYAML(tt.computed) - + result := buildGroupIdentifier(tt.policy, tt.workflowData) if result != tt.expected { - t.Errorf("YAML doesn't match.\nGot:\n%s\nExpected:\n%s", result, tt.expected) - } - }) - } -} - -func TestMergePolicySets(t *testing.T) { - tests := []struct { - name string - base *ConcurrencyPolicySet - override *ConcurrencyPolicySet - expected *ConcurrencyPolicySet - description string - }{ - { - name: "merge with nil override", - base: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{Group: "workflow"}, - Custom: make(map[string]*ConcurrencyPolicy), - }, - override: nil, - expected: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{Group: "workflow"}, - Custom: make(map[string]*ConcurrencyPolicy), - }, - description: "Merge with nil should return base", - }, - { - name: "override default policy", - base: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{Group: "workflow"}, - Custom: make(map[string]*ConcurrencyPolicy), - }, - override: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "custom-workflow", - CancelInProgress: boolPtr(true), - }, - Custom: make(map[string]*ConcurrencyPolicy), - }, - expected: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "custom-workflow", - CancelInProgress: boolPtr(true), - }, - Custom: make(map[string]*ConcurrencyPolicy), - }, - description: "Override should replace default policy", - }, - { - name: "partial override", - base: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "workflow", - Node: "base-node", - }, - Custom: make(map[string]*ConcurrencyPolicy), - }, - override: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - CancelInProgress: boolPtr(true), - }, - Custom: make(map[string]*ConcurrencyPolicy), - }, - expected: &ConcurrencyPolicySet{ - Default: &ConcurrencyPolicy{ - Group: "workflow", - Node: "base-node", - CancelInProgress: boolPtr(true), - }, - Custom: make(map[string]*ConcurrencyPolicy), - }, - description: "Partial override should merge fields", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := mergePolicySets(tt.base, tt.override) - - if !comparePolicySets(result, tt.expected) { - t.Errorf("Merged policy sets don't match.\nGot: %+v\nExpected: %+v", result, tt.expected) + t.Errorf("Group identifier mismatch.\nGot: %s\nExpected: %s", result, tt.expected) } }) } } -// Helper functions for testing - -func boolPtr(b bool) *bool { - return &b -} - -func comparePolicySets(a, b *ConcurrencyPolicySet) bool { - if a == nil && b == nil { - return true - } - if a == nil || b == nil { - return false - } - - return comparePolicies(a.Default, b.Default) && - comparePolicies(a.Issues, b.Issues) && - comparePolicies(a.PullRequest, b.PullRequest) && - comparePolicies(a.Schedule, b.Schedule) && - comparePolicies(a.Manual, b.Manual) && - compareCustomPolicies(a.Custom, b.Custom) -} +// Helper functions -func comparePolicies(a, b *ConcurrencyPolicy) bool { - if a == nil && b == nil { - return true - } - if a == nil || b == nil { - return false - } - - return a.Group == b.Group && - a.Node == b.Node && - compareBoolPtrs(a.CancelInProgress, b.CancelInProgress) -} - -func compareBoolPtrs(a, b *bool) bool { +func compareCancelInProgress(a, b *bool) bool { if a == nil && b == nil { return true } @@ -471,29 +304,3 @@ func compareBoolPtrs(a, b *bool) bool { } return *a == *b } - -func compareCustomPolicies(a, b map[string]*ConcurrencyPolicy) bool { - if len(a) != len(b) { - return false - } - - for k, v := range a { - if !comparePolicies(v, b[k]) { - return false - } - } - - return true -} - -func compareComputedPolicies(a, b *ComputedConcurrencyPolicy) bool { - if a == nil && b == nil { - return true - } - if a == nil || b == nil { - return false - } - - return a.Group == b.Group && - compareBoolPtrs(a.CancelInProgress, b.CancelInProgress) -}