-
Notifications
You must be signed in to change notification settings - Fork 105
Aggregate validation errors across workflow compilation passes #13810
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 all commits
7c9f047
532cc1d
ea410ce
aaacad8
b592253
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,18 +31,29 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str | |
| currentWorkflowName := getCurrentWorkflowName(workflowPath) | ||
| dispatchWorkflowValidationLog.Printf("Current workflow name: %s", currentWorkflowName) | ||
|
|
||
| // Collect all validation errors using ErrorCollector | ||
| collector := NewErrorCollector(c.failFast) | ||
|
|
||
| for _, workflowName := range config.Workflows { | ||
| dispatchWorkflowValidationLog.Printf("Validating workflow: %s", workflowName) | ||
|
|
||
| // Check for self-reference | ||
| if workflowName == currentWorkflowName { | ||
| return fmt.Errorf("dispatch-workflow: self-reference not allowed (workflow '%s' cannot dispatch itself)", workflowName) | ||
| selfRefErr := fmt.Errorf("dispatch-workflow: self-reference not allowed (workflow '%s' cannot dispatch itself)", workflowName) | ||
| if returnErr := collector.Add(selfRefErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
| } | ||
|
|
||
| // Find the workflow file in multiple locations | ||
| fileResult, err := findWorkflowFile(workflowName, workflowPath) | ||
| if err != nil { | ||
| return fmt.Errorf("dispatch-workflow: error finding workflow '%s': %w", workflowName, err) | ||
| findErr := fmt.Errorf("dispatch-workflow: error finding workflow '%s': %w", workflowName, err) | ||
| if returnErr := collector.Add(findErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
|
|
||
| // Check if any workflow file exists | ||
|
|
@@ -53,8 +64,12 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str | |
| repoRoot := filepath.Dir(githubDir) | ||
| workflowsDir := filepath.Join(repoRoot, ".github", "workflows") | ||
|
|
||
| return fmt.Errorf("dispatch-workflow: workflow '%s' not found in %s (tried .md, .lock.yml, and .yml extensions)", | ||
| notFoundErr := fmt.Errorf("dispatch-workflow: workflow '%s' not found in %s (tried .md, .lock.yml, and .yml extensions)", | ||
| workflowName, workflowsDir) | ||
| if returnErr := collector.Add(notFoundErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
|
|
||
| // Validate that the workflow supports workflow_dispatch | ||
|
|
@@ -67,29 +82,49 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str | |
| workflowFile = fileResult.lockPath | ||
| workflowContent, readErr = os.ReadFile(fileResult.lockPath) // #nosec G304 -- Path is validated via isPathWithinDir in findWorkflowFile | ||
| if readErr != nil { | ||
| return fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.lockPath, readErr) | ||
| fileReadErr := fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.lockPath, readErr) | ||
| if returnErr := collector.Add(fileReadErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
| } else if fileResult.ymlExists { | ||
| workflowFile = fileResult.ymlPath | ||
| workflowContent, readErr = os.ReadFile(fileResult.ymlPath) // #nosec G304 -- Path is validated via isPathWithinDir in findWorkflowFile | ||
| if readErr != nil { | ||
| return fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.ymlPath, readErr) | ||
| fileReadErr := fmt.Errorf("dispatch-workflow: failed to read workflow file %s: %w", fileResult.ymlPath, readErr) | ||
| if returnErr := collector.Add(fileReadErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
| } else { | ||
| // Only .md exists - needs to be compiled first | ||
| return fmt.Errorf("dispatch-workflow: workflow '%s' must be compiled first (run 'gh aw compile %s')", workflowName, fileResult.mdPath) | ||
| compileErr := fmt.Errorf("dispatch-workflow: workflow '%s' must be compiled first (run 'gh aw compile %s')", workflowName, fileResult.mdPath) | ||
| if returnErr := collector.Add(compileErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
|
|
||
| // Parse the workflow YAML to check for workflow_dispatch trigger | ||
| var workflow map[string]any | ||
| if err := yaml.Unmarshal(workflowContent, &workflow); err != nil { | ||
| return fmt.Errorf("dispatch-workflow: failed to parse workflow file %s: %w", workflowFile, err) | ||
| parseErr := fmt.Errorf("dispatch-workflow: failed to parse workflow file %s: %w", workflowFile, err) | ||
| if returnErr := collector.Add(parseErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
|
|
||
| // Check if the workflow has an "on" section | ||
| onSection, hasOn := workflow["on"] | ||
| if !hasOn { | ||
| return fmt.Errorf("dispatch-workflow: workflow '%s' does not have an 'on' trigger section", workflowName) | ||
| onSectionErr := fmt.Errorf("dispatch-workflow: workflow '%s' does not have an 'on' trigger section", workflowName) | ||
| if returnErr := collector.Add(onSectionErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
|
|
||
| // Check if workflow_dispatch is in the "on" section | ||
|
|
@@ -114,14 +149,20 @@ func (c *Compiler) validateDispatchWorkflow(data *WorkflowData, workflowPath str | |
| } | ||
|
|
||
| if !hasWorkflowDispatch { | ||
| return fmt.Errorf("dispatch-workflow: workflow '%s' does not support workflow_dispatch trigger (must include 'workflow_dispatch' in the 'on' section)", workflowName) | ||
| dispatchErr := fmt.Errorf("dispatch-workflow: workflow '%s' does not support workflow_dispatch trigger (must include 'workflow_dispatch' in the 'on' section)", workflowName) | ||
| if returnErr := collector.Add(dispatchErr); returnErr != nil { | ||
| return returnErr // Fail-fast mode | ||
| } | ||
| continue // Skip further validation for this workflow | ||
|
||
| } | ||
|
|
||
| dispatchWorkflowValidationLog.Printf("Workflow '%s' is valid for dispatch (found in %s)", workflowName, workflowFile) | ||
| } | ||
|
|
||
| dispatchWorkflowValidationLog.Printf("All %d workflows validated successfully", len(config.Workflows)) | ||
| return nil | ||
| dispatchWorkflowValidationLog.Printf("Dispatch workflow validation completed: error_count=%d, total_workflows=%d", collector.Count(), len(config.Workflows)) | ||
|
|
||
| // Return aggregated errors with formatted output | ||
| return collector.FormattedError("dispatch-workflow") | ||
| } | ||
|
|
||
| // extractWorkflowDispatchInputs parses a workflow file and extracts the workflow_dispatch inputs schema | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -111,6 +111,9 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error | |||||
|
|
||||||
| repositoryFeaturesLog.Printf("Checking repository features for: %s", repo) | ||||||
|
|
||||||
| // Collect all validation errors using ErrorCollector | ||||||
| collector := NewErrorCollector(c.failFast) | ||||||
|
|
||||||
| // Check if discussions are enabled when create-discussion or add-comment with discussion: true is configured | ||||||
| needsDiscussions := workflowData.SafeOutputs.CreateDiscussions != nil || | ||||||
| (workflowData.SafeOutputs.AddComments != nil && | ||||||
|
|
@@ -128,10 +131,8 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error | |||||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage( | ||||||
| fmt.Sprintf("Could not verify if discussions are enabled: %v", err))) | ||||||
| } | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| if !hasDiscussions { | ||||||
| // Continue checking other features even if this check fails | ||||||
| } else if !hasDiscussions { | ||||||
| // Changed to warning instead of error per issue feedback | ||||||
| // Strategy: Always try to create the discussion at runtime and investigate if it fails | ||||||
| // The runtime create_discussion handler will provide better error messages if creation fails | ||||||
|
|
@@ -146,7 +147,7 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error | |||||
| if c.verbose { | ||||||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(warningMsg)) | ||||||
| } | ||||||
| // Don't return error - allow compilation to proceed and let runtime handle the actual attempt | ||||||
| // Don't add to error collector - this is a warning, not an error | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -161,15 +162,19 @@ func (c *Compiler) validateRepositoryFeatures(workflowData *WorkflowData) error | |||||
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage( | ||||||
| fmt.Sprintf("Could not verify if issues are enabled: %v", err))) | ||||||
| } | ||||||
| return nil | ||||||
| } | ||||||
|
|
||||||
| if !hasIssues { | ||||||
| return fmt.Errorf("workflow uses safe-outputs.create-issue but repository %s does not have issues enabled. Enable issues in repository settings or remove create-issue from safe-outputs", repo) | ||||||
| // Continue to return aggregated errors even if this check fails | ||||||
|
||||||
| // Continue to return aggregated errors even if this check fails | |
| // Continue checking other features even if this check fails |
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.
Multiple identical comments 'Skip further validation for this workflow' repeated throughout the function create noise. Consider reducing these comments or making them more specific to each validation step (e.g., 'Skip after self-reference check', 'Skip after file read error').