feat/support layers #2061
feat/support layers #2061
Conversation
motatoes
commented
Jul 30, 2025
motatoes
commented
- support layers in digger.yml
- initial layers summary comment
- support initial layers summary
- checkpoint
Summary of Bugs Found in CodebaseTotal bugs found: 2 Critical Bugs
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR introduces a comprehensive layers feature for Digger, allowing infrastructure projects to be organized into execution layers for ordered deployment. The implementation spans across multiple components:
Configuration Support: New fields are added to the YAML configuration - RespectLayers as a global boolean flag to enable the feature, and Layer as a uint field on individual projects to specify their execution layer (defaulting to 0).
Database Schema: A new migration adds a layer column to the digger_batches table to persist layer information for batch processing.
Job Scheduling: The Job struct gains a Layer field, and a new CountUniqueLayers utility function helps analyze layer distribution across job collections. The scheduler also includes ParseProjectLayer functionality to extract layer specifications from user comments using --layer N syntax.
CI Integration: All CI backends (GitHub, GitLab, Bitbucket) are updated to handle the new layer information. The ProcessIssueCommentEvent function now returns an additional boolean indicating when a specific layer is requested, enabling layer-based project filtering.
User Interface: The PR comment system is enhanced with a new ReportLayersTableForJobs function that displays projects organized by layer instead of the previous status-based reporting. This provides users with clear visibility into which projects belong to which layer.
Execution Logic: When RespectLayers is enabled and multiple layers are detected, the system displays a layers summary table and stops automatic execution, allowing users to manually control deployment order layer by layer.
The feature integrates with existing Terragrunt configurations by mapping execution order groups to layers, and maintains backward compatibility through optional configuration fields and feature flags.
Confidence score: 3/5
• This PR has some implementation gaps and potential issues that need attention before merging safely.
• The score reflects incomplete error handling, missing field serialization, placeholder code, and some inconsistent implementation patterns that could cause runtime issues.
• Files needing more attention: backend/controllers/github.go (incomplete conditional block), backend/models/scheduler.go (missing Layer serialization), and libs/ci/generic/events.go (layer filtering logic needs validation)
Context used:
Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)
16 files reviewed, 8 comments
| @@ -190,39 +196,61 @@ func ReportInitialJobsStatus(cr *CommentReporter, jobs []scheduler.Job) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func ReportNoProjectsImpacted(cr *CommentReporter, jobs []scheduler.Job) error { | |||
| func ReportLayersTableForJobs(cr *CommentReporter, jobs []scheduler.Job) error { | |||
There was a problem hiding this comment.
style: Function name suggests it reports layers table but the log message still says 'Reporting initial jobs status' - should be updated for consistency
| return jobs[i].Layer < jobs[j].Layer | ||
| }) | ||
|
|
||
| slog.Info("Reporting initial jobs status", |
There was a problem hiding this comment.
style: Log message is inconsistent with function purpose - should say 'Reporting layers table' instead of 'initial jobs status'
| slog.Info("Reporting initial jobs status", | |
| slog.Info("Reporting layers table for jobs", |
Context Used: Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)
| err := prService.EditComment(prNumber, commentId, message) | ||
| if err != nil { | ||
| slog.Error("Failed to update comment with no projects impacted message", | ||
| slog.Error("Failed to update comment with initial jobs status", |
There was a problem hiding this comment.
style: Error log message is generic 'initial jobs status' but should reflect the layers table context
| slog.Error("Failed to update comment with initial jobs status", | |
| slog.Error("Failed to update comment with layers table", |
Context Used: Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
| "prNumber", prNumber, | ||
| "commentId", commentId, | ||
| "error", err, | ||
| ) | ||
| return err | ||
| } | ||
|
|
||
| slog.Debug("Successfully reported no projects impacted", "prNumber", prNumber, "commentId", commentId) | ||
| slog.Debug("Successfully reported initial jobs status", "prNumber", prNumber, "commentId", commentId) |
There was a problem hiding this comment.
style: Success log message should reflect the actual operation being performed (layers table) rather than generic 'initial jobs status'
| slog.Debug("Successfully reported initial jobs status", "prNumber", prNumber, "commentId", commentId) | |
| slog.Debug("Successfully reported layers table", "prNumber", prNumber, "commentId", commentId) |
Context Used: Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)
| if config.RespectLayers { | ||
|
|
||
| } |
There was a problem hiding this comment.
logic: Empty conditional block - this appears to be unfinished code that should be removed or completed
libs/ci/generic/events.go
Outdated
| // if a layer is requested we filter out that layer only and return those layers | ||
| requestedLayer, layerFound, err := scheduler.ParseProjectLayer(commentBody) | ||
| if err != nil { | ||
| return nil, nil, nil, 0, false, fmt.Errorf("failed to parse layer from comment %v", err) |
There was a problem hiding this comment.
style: Consider enriching this error message with the prNumber context for better debugging
Context Used: Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
| @@ -0,0 +1,2 @@ | |||
| -- Modify "digger_batches" table | |||
| ALTER TABLE "public"."digger_batches" ADD COLUMN "layer" bigint NULL; | |||
There was a problem hiding this comment.
style: Consider using int instead of bigint for the layer column since layers are typically small sequential numbers (0, 1, 2, etc.) and the Go model uses uint which maps better to int
![bismuthdev[bot]](https://avatars.githubusercontent.com/in/957724?s=60&v=4){kind=small}
| if config.RespectLayers && nLayers > 1 { | ||
| slog.Debug("Respecting layers", | ||
| "prNumber", prNumber) | ||
| err = utils.ReportLayersTableForJobs(commentReporter, jobsForImpactedProjects) | ||
| if err != nil { | ||
| slog.Error("Failed to comment initial status for jobs", | ||
| "prNumber", prNumber, | ||
| "jobCount", len(jobsForImpactedProjects), | ||
| "error", err, | ||
| ) | ||
| commentReporterManager.UpdateComment(fmt.Sprintf(":x: Failed to comment initial status for jobs: %v", err)) | ||
| return fmt.Errorf("failed to comment initial status for jobs") | ||
| } | ||
| slog.Debug("not performing plan since there are multiple layers and respect_layers is enabled") | ||
| return nil |
There was a problem hiding this comment.
The current implementation of the respect_layers feature silently returns early when multiple layers are detected, without providing clear feedback to the user about why no jobs are being triggered. This can be confusing for users who expect automatic job triggering.
The bug is that while the code does report a layers table with instructions in a collapsible details section, it doesn't explicitly inform users that no automatic job triggering will occur. This significant change in behavior should be prominently communicated.
The fix adds a more visible message explaining that when respect_layers is enabled with multiple layers, jobs aren't automatically triggered, and directs users to the instructions for manually triggering jobs by layer. It also improves the logging from debug to info level for better visibility in logs.
| if config.RespectLayers && nLayers > 1 { | |
| slog.Debug("Respecting layers", | |
| "prNumber", prNumber) | |
| err = utils.ReportLayersTableForJobs(commentReporter, jobsForImpactedProjects) | |
| if err != nil { | |
| slog.Error("Failed to comment initial status for jobs", | |
| "prNumber", prNumber, | |
| "jobCount", len(jobsForImpactedProjects), | |
| "error", err, | |
| ) | |
| commentReporterManager.UpdateComment(fmt.Sprintf(":x: Failed to comment initial status for jobs: %v", err)) | |
| return fmt.Errorf("failed to comment initial status for jobs") | |
| } | |
| slog.Debug("not performing plan since there are multiple layers and respect_layers is enabled") | |
| return nil | |
| if config.RespectLayers && nLayers > 1 { | |
| slog.Debug("Respecting layers", | |
| "prNumber", prNumber) | |
| err = utils.ReportLayersTableForJobs(commentReporter, jobsForImpactedProjects) | |
| if err != nil { | |
| slog.Error("Failed to comment initial status for jobs", | |
| "prNumber", prNumber, | |
| "jobCount", len(jobsForImpactedProjects), | |
| "error", err, | |
| ) | |
| commentReporterManager.UpdateComment(fmt.Sprintf(":x: Failed to comment initial status for jobs: %v", err)) | |
| return fmt.Errorf("failed to comment initial status for jobs") | |
| } | |
| slog.Info("Not automatically triggering jobs since there are multiple layers and respect_layers is enabled", | |
| "prNumber", prNumber, | |
| "nLayers", nLayers) | |
| commentReporterManager.UpdateComment(fmt.Sprintf(":information_source: **Multiple layers detected (%d).** When `respect_layers` is enabled, jobs are not automatically triggered. Please use the layer-specific commands shown in the instructions above to manually trigger plans and applies for each layer.", nLayers)) | |
| return nil |
libs/digger_config/digger_config.go
Outdated
| executionOrderGroups := true | ||
| if parsingConfig.ExecutionOrderGroups != nil { | ||
| executionOrderGroups = *parsingConfig.ExecutionOrderGroups | ||
| } |
There was a problem hiding this comment.
The default value for executionOrderGroups has been changed from false to true in the terragrunt parsing configuration. This is a significant change that could affect existing users who rely on the previous default behavior.
The executionOrderGroups parameter controls how Terragrunt projects are organized and executed. Changing the default from false to true means that all users who don't explicitly set this parameter will now have a different execution order for their Terragrunt projects, which could lead to unexpected behavior in their infrastructure deployments.
This type of change should be made with caution and properly documented as it represents a breaking change for users who rely on the previous default behavior.
| executionOrderGroups := true | |
| if parsingConfig.ExecutionOrderGroups != nil { | |
| executionOrderGroups = *parsingConfig.ExecutionOrderGroups | |
| } | |
| executionOrderGroups := false | |
| if parsingConfig.ExecutionOrderGroups != nil { | |
| executionOrderGroups = *parsingConfig.ExecutionOrderGroups | |
| } |
There was a problem hiding this comment.
Greptile Summary
This PR implements support for layers in Digger configuration and job processing. The primary change is adding a Layer field to the Job struct to track which layer a job belongs to as part of the layers feature implementation.
The specific changes in this file focus on updating the test infrastructure to accommodate the new Layer field. Two minor formatting improvements remove extra spaces from comma-separated lists in the fieldsToIgnore arrays. More importantly, the Layer field is added to the exclusion list in the TestAllFieldsInJobAreAlsoInJobJson function, indicating that this field is intentionally not included in the JobJson struct used for serialization to external systems.
This change suggests that Layer is treated as a server-side organizational concept that doesn't need to be transmitted to job runners or external systems. The test exclusion prevents validation failures when comparing Job and JobJson struct fields, allowing the layers feature to be integrated without breaking existing serialization contracts.
Confidence score: 2/5
- This change introduces a critical data consistency issue that could cause production problems
- The Layer field is excluded from serialization validation but the JobToJson/JsonToJob conversion functions don't handle it, leading to potential data loss
- The libs/scheduler/json_models_test.go file needs immediate attention to ensure proper handling of the Layer field in conversion functions
Context used:
Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)
1 file reviewed, no comments
|
Bismuth completed its review and found no issues. |
|
Cancelled. |
There was a problem hiding this comment.
Greptile Summary
This PR implements layer support in Digger's CI/CD workflow, allowing users to organize and execute Terraform projects in hierarchical layers. The changes refactor the event processing system to support layer-based project filtering and execution.
Key Changes:
-
Event Processing Refactor: The
ProcessIssueCommentEventfunction now returns a structuredProcessIssueCommentEventResultobject instead of multiple return values, containing bothImpactedProjectsForComment(filtered by layer) andAllImpactedProjects(all projects impacted by PR changes). -
Layer Parsing: New functionality parses layer specifications from PR comments using
scheduler.ParseProjectLayer(), enabling commands likedigger plan layer=1to target specific layers. -
Cross-Platform Support: The refactoring is consistently applied across all VCS integrations (GitHub, GitLab, and Bitbucket controllers), ensuring uniform layer support across platforms.
-
Job Creation Updates: The
ConvertIssueCommentEventToJobsfunction signature is updated to accept both project sets, allowing proper distinction between projects that should execute versus all impacted projects for reporting. -
Layer Table Reporting: Integration with
ReportLayersTableForJobsfunctionality provides users with layer-specific deployment instructions when multiple layers are detected.
The layer functionality enables users to deploy interdependent Terraform projects in the correct order, which is essential for complex infrastructure deployments where some projects depend on outputs from others. For example, foundational infrastructure (layer 0) can be deployed before application resources (layer 1).
Confidence score: 3/5
- This PR introduces significant architectural changes that affect core event processing logic across multiple controllers
- The refactoring from multiple return values to a structured result object is a breaking change that could impact other parts of the codebase
- There are several unfinished implementation details, including an empty conditional block in
backend/controllers/github.goand inconsistent logging messages that reference outdated functionality
Context used:
Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)
4 files reviewed, 1 comment
| @@ -400,17 +400,20 @@ func handleIssueCommentEvent(gitlabProvider utils.GitlabProvider, payload *gitla | |||
| return fmt.Errorf("error while fetching branch name") | |||
| } | |||
|
|
|||
| impactedProjects, impactedProjectsSourceMapping, requestedProject, _, err := generic.ProcessIssueCommentEvent(issueNumber, commentBody, config, projectsGraph, glService) | |||
| processIssueCommentEventResult, err := generic.ProcessIssueCommentEvent(issueNumber, commentBody, config, projectsGraph, glService) | |||
| if err != nil { | |||
| log.Printf("Error processing event: %v", err) | |||
| utils.InitCommentReporter(glService, issueNumber, fmt.Sprintf(":x: Error processing event: %v", err)) | |||
| return fmt.Errorf("error processing event") | |||
| } | |||
| log.Printf("GitHub IssueComment event processed successfully\n") | |||
There was a problem hiding this comment.
style: Log message mentions 'GitHub IssueComment event' but this is GitLab controller - should say 'GitLab IssueComment event'
| log.Printf("GitHub IssueComment event processed successfully\n") | |
| log.Printf("GitLab IssueComment event processed successfully\n") |
Summary of Bugs Found in CodebaseA total of 4 bugs were identified in the codebase: Critical Bugs
|
There was a problem hiding this comment.
Greptile Summary
This PR introduces support for layers in Digger, which is a feature that enables hierarchical project organization and execution ordering. The changes primarily focus on updating function signatures and test code to accommodate the new layers functionality.
The main changes modify the ConvertIssueCommentEventToJobs function signature in both test files and integration tests. Previously, this function took separate impactedProjects and requestedProject parameters, but with layers support, it now takes impactedProjects twice as the 5th and 6th parameters. This suggests that the layers feature unified the concept of impacted and requested projects - when layers are involved, all impacted projects need to be processed together as a cohesive unit rather than distinguishing between different project types.
The integration with the existing codebase shows that layers functionality builds on top of the existing project dependency and job scheduling system. From the RAG context, we can see that the CI system already handles project dependencies through dependency graphs and converts events into scheduler jobs. The layers feature appears to add another dimension to this processing, where projects can be organized into execution layers that respect dependencies and execution order.
Confidence score: 4/5
• This PR is fairly safe to merge as it only modifies test files and function parameters without changing core business logic
• The score reflects that these are test-only changes with consistent patterns, though the actual layers implementation isn't visible in these files
• Files that need more attention: None - these are straightforward test adaptations
2 files reviewed, no comments
| @@ -251,7 +255,7 @@ func handleIssueCommentEventBB(bitbucketProvider utils.BitbucketProvider, payloa | |||
| return nil | |||
| } | |||
|
|
|||
| jobs, _, err := generic.ConvertIssueCommentEventToJobs(repoFullName, actor, issueNumber, commentBody, impactedProjects, requestedProject, config.Workflows, prBranchName, defaultBranch) | |||
| jobs, _, err := generic.ConvertIssueCommentEventToJobs(repoFullName, actor, issueNumber, commentBody, impactedProjectsForComment, allImpactedProjects, config.Workflows, prBranchName, defaultBranch) | |||
There was a problem hiding this comment.
In ee/backend/controllers/bitbucket.go, the ConvertIssueCommentEventToJobs function returns three values, but the second return value is being ignored with an underscore. This second return value is coverAllImpactedProjects, which is important for determining if all impacted projects should be covered.
Later in the code at line 302, this value is hardcoded to true when calling ConvertJobsToDiggerJobs:
batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSBitbucket, organisationId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, 0, branch, issueNumber, repoOwner, repoName, repoFullName, commitSha, commentId64, diggerYmlStr, 0, "", false, true, vcsConnectionId)This is inconsistent with how the function is used in the GitLab controller, where the actual returned value is passed to ConvertJobsToDiggerJobs. For consistency and correctness, the code should capture the coverAllImpactedProjects value and use it instead of hardcoding it to true.
| jobs, _, err := generic.ConvertIssueCommentEventToJobs(repoFullName, actor, issueNumber, commentBody, impactedProjectsForComment, allImpactedProjects, config.Workflows, prBranchName, defaultBranch) | |
| jobs, coverAllImpactedProjects, err := generic.ConvertIssueCommentEventToJobs(repoFullName, actor, issueNumber, commentBody, impactedProjectsForComment, allImpactedProjects, config.Workflows, prBranchName, defaultBranch) |
There was a problem hiding this comment.
Greptile Summary
This PR introduces documentation for the new layering feature in Digger that supports Terragrunt dependency management. The changes add a new documentation page that explains how to use the respect_layers configuration option to execute Terragrunt modules in dependency order automatically.
The documentation is integrated into the existing docs structure by adding a navigation entry in mint.json under the Features section. The new MDX file (docs/ce/features/layering-terragrunt.mdx) provides comprehensive coverage of the layering feature, including configuration examples, workflow explanations, and visual aids to help users understand how to enable and use this functionality.
This documentation supports the broader layering feature implementation that allows users to avoid the cumbersome manual process of applying changes in correct dependency order using individual -p flags. Instead, users can enable respect_layers: true in their digger.yml configuration to have Digger automatically handle the execution order based on Terragrunt dependencies.
PR Description Notes:
- The PR body contains very brief bullet points that don't clearly explain the changes
- Consider expanding the description to better communicate the scope and impact of the new layering feature
Confidence score: 4/5
- This PR is generally safe to merge as it only adds documentation without affecting core functionality
- Score reduced by 1 point due to multiple spelling and grammatical errors in the documentation that impact professionalism
- The documentation file
docs/ce/features/layering-terragrunt.mdxneeds proofreading to fix typos like 'Layring' instead of 'Layering', 'dependant' instead of 'dependent', 'dependend' instead of 'depended', and 'appiles' instead of 'applies'
2 files reviewed, no comments
| @@ -0,0 +1,54 @@ | |||
| --- | |||
| title: "Layring with Terragrunt" | |||
There was a problem hiding this comment.
syntax: Title has spelling error: 'Layring' should be 'Layering'
| title: "Layring with Terragrunt" | |
| title: "Layering with Terragrunt" |
| title: "Layring with Terragrunt" | ||
| --- | ||
|
|
||
| In a terragrunt monorepo you will usually have multiple dependant modules |
There was a problem hiding this comment.
syntax: Spelling error: 'dependant' should be 'dependent'
| In a terragrunt monorepo you will usually have multiple dependant modules | |
| In a terragrunt monorepo you will usually have multiple dependent modules |
|
|
||
| In a terragrunt monorepo you will usually have multiple dependant modules | ||
| where the order of execution is important. The dependencies can be represented as rings or layers | ||
| where the innermost layer is dependend on by the layer above which is depended on by the layer above and so on. |
There was a problem hiding this comment.
syntax: Spelling error: 'dependend' should be 'depended'
| where the innermost layer is dependend on by the layer above which is depended on by the layer above and so on. | |
| where the innermost layer is depended on by the layer above which is depended on by the layer above and so on. |
|
|
||
| Finally we want to plan and apply the modules in the layer2 group since they depend on the modules in layer1. | ||
|
|
||
| In digger this could have been done individually by using the -p flag such as "digger apply -p core_network" and doing the appiles in the intended order, but this was cumbersome and tiring. |
There was a problem hiding this comment.
syntax: Spelling error: 'appiles' should be 'applies'
| In digger this could have been done individually by using the -p flag such as "digger apply -p core_network" and doing the appiles in the intended order, but this was cumbersome and tiring. | |
| In digger this could have been done individually by using the -p flag such as "digger apply -p core_network" and doing the applies in the intended order, but this was cumbersome and tiring. |
Bug SummaryA total of 2 bugs were found in the codebase:
The second bug is more critical as it could unexpectedly change system behavior for existing users, potentially disrupting their workflows. |
There was a problem hiding this comment.
Greptile Summary
This PR introduces layer support for Digger, implementing a Terragrunt-style dependency management system where infrastructure projects can be organized into execution layers (0, 1, 2, etc.). The feature addresses the challenge of managing complex monorepos where modules have hierarchical dependencies that must be executed in a specific order.
The implementation adds several key components:
Configuration Layer: New fields RespectLayers (bool) in DiggerConfig and Layer (uint) in Project structs allow users to enable layering globally and assign layer numbers to individual projects in their digger.yml configuration.
Database Schema: A migration adds a layer column to the digger_batches table to persist layer information alongside batch metadata.
Comment Processing: The ProcessIssueCommentEvent function now supports parsing --layer N flags from comments, enabling users to target specific layers for execution. The function returns a structured result distinguishing between projects filtered for the current comment versus all impacted projects.
Job Scheduling: The Job struct gains a Layer field, and utility functions like CountUniqueLayers help analyze layer distribution across job batches.
UI Integration: When multiple layers are detected and respect_layers is enabled, the system shows a layers table instead of automatically triggering jobs, allowing manual layer-by-layer execution using commands like digger plan --layer 0.
Cross-Platform Support: The changes are implemented consistently across GitHub, GitLab, and Bitbucket integrations.
The feature integrates with existing Terragrunt parsing logic, defaulting executionOrderGroups to true and mapping Terragrunt's ExecutionOrderGroup values to Digger's layer system. This enables automatic dependency analysis while giving users fine-grained control over execution sequencing.
Confidence score: 2/5
• This PR has critical serialization issues that will cause data loss and break the layers functionality
• Multiple components are missing Layer field support in JSON serialization, creating inconsistent data handling
• The JobJson struct lacks the Layer field, causing data loss during Job-to-JSON conversion which is used throughout the system
Context used:
Context - Enrich debug logs with relevant context. Always include identifiers like 'prNumber' in logs to facilitate easier tracing and troubleshooting. (link)
Context - Use consistent and descriptive keys for debug logging. For example, prefer 'prBatchCount' over 'len(prBatches)' to maintain clarity and adhere to naming conventions. (link)
21 files reviewed, 2 comments
| func ParseProjectLayer(comment string) (int, bool, error) { | ||
| re := regexp.MustCompile(`--layer ([0-9]+)`) | ||
| match := re.FindStringSubmatch(comment) | ||
| if len(match) > 1 { | ||
| layerInt, err := strconv.Atoi(match[1]) | ||
| if err != nil { | ||
| return -1, false, err | ||
| } | ||
| return layerInt, true, nil | ||
|
|
||
| } | ||
| return -1, false, nil | ||
| } |
There was a problem hiding this comment.
logic: Type inconsistency: function returns int but Job.Layer field is uint - consider returning uint for consistency
| @@ -495,6 +495,7 @@ func ConvertGithubPullRequestEventToJobs(payload *github.PullRequestEvent, impac | |||
| ProjectDir: project.Dir, | |||
| ProjectWorkspace: project.Workspace, | |||
| ProjectWorkflow: project.Workflow, | |||
| Layer: project.Layer, | |||
There was a problem hiding this comment.
logic: Layer field missing from JobJson struct - this will cause data loss during Job to JobJson conversion via JobToJson() function
| @@ -788,6 +811,9 @@ func handlePullRequestEvent(gh utils.GithubClientProvider, payload *github.PullR | |||
| "jobCount", len(impactedJobsMap), | |||
| ) | |||
|
|
|||
| if config.RespectLayers { | |||
There was a problem hiding this comment.
There's an empty if block at line 814 that appears to be incomplete code. The condition if config.RespectLayers { } doesn't do anything. This is likely a duplicate of the proper conditional check that appears a few lines later at line 443 where the same condition is properly implemented with a body. The empty if block should be removed or properly implemented. Based on the context and the code at line 443, this appears to be a mistake where the same condition was accidentally duplicated without its implementation.
| if config.RespectLayers { | |
| if config.RespectLayers && nLayers > 1 { |
libs/digger_config/digger_config.go
Outdated
| executionOrderGroups := true | ||
| if parsingConfig.ExecutionOrderGroups != nil { | ||
| executionOrderGroups = *parsingConfig.ExecutionOrderGroups | ||
| } |
There was a problem hiding this comment.
The default value of executionOrderGroups was changed from false to true in the terragrunt parsing configuration. This change could affect existing configurations that rely on the previous default behavior.
The executionOrderGroups parameter controls whether projects are organized into layers for execution ordering. When set to true, it enables the layering feature which affects how projects are executed in a specific order based on dependencies.
This change is potentially breaking for existing users who haven't explicitly set this parameter in their configuration, as it would suddenly change the execution behavior of their terragrunt projects. The layering feature should be opt-in rather than enabled by default to maintain backward compatibility.
The documentation in docs/ce/features/layering-terragrunt.mdx shows that users need to explicitly enable this feature with executionOrderGroups: true in their configuration, which indicates this should not be the default.
| executionOrderGroups := true | |
| if parsingConfig.ExecutionOrderGroups != nil { | |
| executionOrderGroups = *parsingConfig.ExecutionOrderGroups | |
| } | |
| executionOrderGroups := false | |
| if parsingConfig.ExecutionOrderGroups != nil { | |
| executionOrderGroups = *parsingConfig.ExecutionOrderGroups | |
| } |
There was a problem hiding this comment.
Greptile Summary
This PR introduces support for layers functionality in the Digger CI system by modifying how impacted projects are passed to the ConvertIssueCommentEventToJobs function in cli/pkg/github/github.go.
Changes Made
The key change is in lines 293-299 where a new variable impactedProjectsForEvent is introduced:
var impactedProjectsForEvent []digger_config.Project
if requestedProject != nil {
impactedProjectsForEvent = []digger_config.Project{*requestedProject}
} else {
impactedProjectsForEvent = impactedProjects
}
jobs, coversAllImpactedProjects, err = generic.ConvertIssueCommentEventToJobs(repoFullName, requestedBy, prNumber, commentBody, impactedProjectsForEvent, impactedProjects, diggerConfig.Workflows, prBranchName, defaultBranch)Purpose and Integration
This change enables layer-aware project execution by distinguishing between:
- Projects to process for the specific event (
impactedProjectsForEvent): When a user requests a specific project via comment (e.g.,digger plan -p project_name), only that project is processed - All impacted projects (
impactedProjects): The full scope of projects affected by the PR, maintained for coverage analysis
This modification is essential for the layers feature in Terragrunt monorepos where projects have dependencies and need execution in a specific order. The system now needs to understand both what the user specifically requested and the full impact scope to make informed decisions about execution order and coverage.
The change integrates with the existing CI event processing pipeline by extending the ConvertIssueCommentEventToJobs function signature to accept both parameters, allowing downstream logic to handle layer-aware execution while maintaining backward compatibility.
Confidence score: 4/5
- This change appears safe and follows a logical pattern for supporting layers functionality
- The modification is well-contained and doesn't break existing interfaces
- The code properly handles both cases (with and without requested project) using standard conditional logic
- Files that need more attention: The
generic.ConvertIssueCommentEventToJobsfunction implementation should be reviewed to ensure it properly handles the new dual-parameter approach
1 file reviewed, no comments
Bug SummaryTotal Bugs Found: 1 Critical Bug
|
There was a problem hiding this comment.
Greptile Summary
This PR updates the test file for the layers support feature, specifically modifying how the ConvertIssueCommentEventToJobs function is called in test scenarios. The changes reflect a fundamental architectural shift in how Digger handles project dependencies with the new layers feature.
The key changes include:
- Replacing the
requestedProjectparameter withimpactedProjectsin function calls toConvertIssueCommentEventToJobs - Removing unused variable declarations like
var requestedProject = project - Updating test assertions to work with the new function signature
These modifications align with the broader layers implementation where the concept of a single "requested project" has been replaced with handling multiple "impacted projects" to support layered infrastructure dependencies. In a layered architecture, a single command might need to operate on multiple projects in a specific order based on their dependency relationships, rather than just targeting one project.
The changes ensure that the test suite properly validates the new layers functionality while maintaining compatibility with the updated API. This is part of the larger effort to support layers in digger.yml configuration files, enabling users to define and manage complex infrastructure dependencies through organized layers.
Confidence score: 5/5
- This PR is very safe to merge as it only updates test code to match API changes
- The changes are straightforward parameter updates that align with the layers feature implementation
- No production code files need additional attention - this is purely test maintenance
1 file reviewed, no comments
Bug SummaryTotal Bugs Found: 1 Bug Details
This bug could lead to unexpected behavior in layer-based operations since the layer information would be missing from newly created batches. |
Bug SummaryTotal Bugs Found: 1 Critical Bug
|
There was a problem hiding this comment.
Greptile Summary
This PR implements support for layers in Digger's configuration system, introducing a new way to manage project dependencies and execution order. The primary change modifies the default value of executionOrderGroups from true back to false in the Terragrunt configuration hydration function.
The layering feature allows users to explicitly define project execution order through a Layer field assigned to projects and a new respect_layers configuration option in digger.yml. When respect_layers is enabled and multiple layers are detected, the system reports a layers table with instructions for manual job triggering rather than automatically triggering all jobs.
This change represents a shift from automatic execution order grouping (based on Terragrunt's execution order groups) to explicit layer-based organization. The executionOrderGroups parameter controls whether Terragrunt projects are organized into layers for execution ordering, and setting it to false by default ensures the new layering system takes precedence when configured.
The implementation includes:
- New
Layerfield assignment to projects (set toexecutionOrderGroupvalue) - Support for
respect_layersconfiguration in digger.yml - Layers summary comment functionality for displaying project organization
- Manual job triggering workflow when multiple layers are present
This change integrates with the existing project configuration system and maintains backward compatibility for users who don't use the layering feature, while providing more granular control over project dependencies and execution order for those who opt in.
Confidence score: 4/5
• This PR is generally safe to merge with the default change reverting to expected behavior
• The confidence score reflects that this is part of a larger feature implementation that may need thorough testing
• The libs/digger_config/digger_config.go file needs careful review to ensure the layering implementation is complete
1 file reviewed, no comments
There was a problem hiding this comment.
Greptile Summary
This change adds documentation for using the layering feature with other Digger flags, specifically include_patterns. The documentation provides a practical example showing how to coordinate changes across layers when dependencies exist between modules.
The new section demonstrates a common use case where a VPC module (layer 1) serves as a dependency for multiple service modules (layer 2). By combining respect_layers: true with include_patterns, users can ensure that when changes are made to the VPC module, it will be planned and applied first, followed by the dependent services running in parallel within their layer.
This documentation addition complements the existing layering feature implementation by addressing how layering interacts with other Digger configuration options, filling a gap that users might encounter when trying to coordinate complex multi-module deployments.
PR Description Notes:
- The PR description contains minimal detail and appears to be checkpoint-style notes rather than a comprehensive description of changes
- Consider expanding the description to better explain the documentation addition and its purpose
Confidence score: 4/5
- This documentation addition is safe to merge as it only adds helpful usage examples without changing any code behavior.
- The score is 4/5 due to several spelling and grammatical errors in the added content that should be corrected for professional documentation quality.
- Files needing attention:
docs/ce/features/layering-terragrunt.mdxfor proofreading and corrections
1 file reviewed, no comments
|
Bismuth completed its review and found no issues. |
