fix: auto_merge not working properly with orchestrator backend #1871
Conversation
When running with Digger backend mode, PR is merged when `auto_merge` is enabled even though not all impacted projects have been applied. This is caused because Digger batch does not have context whether all the needed jobs have been triggered when using `digger apply` with `-p` flag. Unlike in backendless mode, where all the jobs are run sequentially and the parent one does have context if all the impacted projects have been applied, in backend mode jobs are triggered via the Batch one, which does have this context. This PR addresses the issue by adding one more column to store this context, which will then be used in the subsequent run.
WalkthroughThe pull request introduces a new boolean parameter Changes
Possibly related issues
Possibly related PRs
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (10)
backend/controllers/github.go (1)
401-401: Consider reducing parameter passing complexity.The introduction of
coverAllImpactedProjectsas an additional return value increases the number of variables passed between functions. Consider grouping related parameters into a struct to enhance code readability and maintainability.Apply this diff to define a struct and update the function signature:
+type EventConversionResult struct { + JobsForImpactedProjects []Job + CoverAllImpactedProjects bool +} -func ConvertGithubPullRequestEventToJobs(...) ([]Job, bool, error) { +func ConvertGithubPullRequestEventToJobs(...) (EventConversionResult, error) { - jobsForImpactedProjects, coverAllImpactedProjects, err := dg_github.ConvertGithubPullRequestEventToJobs(...) + result, err := dg_github.ConvertGithubPullRequestEventToJobs(...)backend/models/storage_test.go (1)
155-155: Add test cases for the newcoverAllImpactedProjectsparameter.The test only verifies batch creation with
coverAllImpactedProjectsset totrue. Consider adding test cases that verify the behavior when this parameter isfalseto ensure both scenarios work correctly.Would you like me to help generate additional test cases to cover both scenarios?
backend/tasks/runs_test.go (1)
147-147: Enhance test coverage for batch creation withcoverAllImpactedProjects.The test should verify that the state machine transitions correctly based on the value of
coverAllImpactedProjects. Consider adding test cases that cover bothtrueandfalsescenarios.Would you like me to help generate additional test cases to verify the state machine behavior with different
coverAllImpactedProjectsvalues?backend/utils/graphs.go (2)
18-18: Consider documenting the new parameter in the function signature.Add a comment explaining the purpose and impact of the
coverAllImpactedProjectsparameter to help future maintainers understand its significance.
47-50: Enhance error handling for batch creation.The error message could be more descriptive. Consider including the value of
coverAllImpactedProjectsin the error message to help with debugging.- return nil, nil, fmt.Errorf("failed to create batch: %v", err) + return nil, nil, fmt.Errorf("failed to create batch (coverAllImpactedProjects=%v): %v", coverAllImpactedProjects, err)ee/backend/hooks/github.go (2)
Line range hint
124-129: Enhance error handling for job conversion.Consider logging the value of
coverAllImpactedProjectswhen an error occurs to help with debugging.if err != nil { - log.Printf("Error converting event to jobs: %v", err) + log.Printf("Error converting event to jobs (coverAllImpactedProjects=%v): %v", coverAllImpactedProjects, err) utils.InitCommentReporter(ghService, issueNumber, fmt.Sprintf(":x: Error converting event to jobs: %v", err)) return fmt.Errorf("error converting event to jobs") }
Line range hint
155-159: Improve error reporting for job conversion.The error message in the comment reporter could be more descriptive to help users understand the issue.
if err != nil { log.Printf("ConvertJobsToDiggerJobs error: %v", err) - utils.InitCommentReporter(ghService, issueNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err)) + utils.InitCommentReporter(ghService, issueNumber, fmt.Sprintf(":x: Failed to process jobs for all impacted projects: %v", err)) return fmt.Errorf("error convertingjobs") }backend/controllers/projects.go (1)
734-734: Simplify boolean comparison and verify auto-merge condition.The boolean comparison can be simplified. Additionally, the new condition ensures safer auto-merge behavior by requiring all impacted projects to be covered.
Apply this diff to simplify the boolean comparison:
-if batch.Status == orchestrator_scheduler.BatchJobSucceeded && batch.BatchType == orchestrator_scheduler.DiggerCommandApply && batch.CoverAllImpactedProjects == true && automerge == true { +if batch.Status == orchestrator_scheduler.BatchJobSucceeded && batch.BatchType == orchestrator_scheduler.DiggerCommandApply && batch.CoverAllImpactedProjects && automerge {🧰 Tools
🪛 golangci-lint (1.62.2)
734-734: S1002: should omit comparison to bool constant, can be simplified to
batch.CoverAllImpactedProjects(gosimple)
backend/controllers/github_test.go (1)
735-735: Add test cases forcoverAllImpactedProjects=false.The test suite only covers scenarios where
coverAllImpactedProjectsistrue. Consider adding test cases that verify the behavior whencoverAllImpactedProjectsisfalse, especially for auto-merge scenarios.Also applies to: 764-764, 797-797, 842-842
ee/backend/controllers/gitlab.go (1)
337-337: Consider improving parameter clarity inConvertJobsToDiggerJobs.The function calls have two boolean parameters next to each other (
false, coverAllImpactedProjects). This could lead to confusion and potential errors when maintaining the code.Consider one of these improvements:
- Use named parameters or a config struct to make the intent clearer
- Reorder parameters to separate the boolean flags
- Add comments explaining the purpose of each boolean parameter
Example with a config struct:
type ConversionConfig struct { IsForceApply bool CoverAllImpactedProjects bool } func ConvertJobsToDiggerJobs( command scheduler.DiggerCommand, // ... other parameters ... - forceApply bool, - coverAllImpactedProjects bool, + config ConversionConfig, ) (uint, error)Also applies to: 528-528
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (12)
backend/controllers/github.go(5 hunks)backend/controllers/github_test.go(5 hunks)backend/controllers/projects.go(2 hunks)backend/controllers/projects_test.go(3 hunks)backend/migrations/20250115155205.sql(1 hunks)backend/models/scheduler.go(2 hunks)backend/models/storage.go(2 hunks)backend/models/storage_test.go(2 hunks)backend/tasks/runs_test.go(2 hunks)backend/utils/graphs.go(2 hunks)ee/backend/controllers/gitlab.go(5 hunks)ee/backend/hooks/github.go(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/migrations/20250115155205.sql
🧰 Additional context used
🪛 golangci-lint (1.62.2)
backend/controllers/projects.go
734-734: S1002: should omit comparison to bool constant, can be simplified to batch.CoverAllImpactedProjects
(gosimple)
🔇 Additional comments (12)
backend/controllers/github.go (4)
9-20: Necessary imports added.The added import statements are necessary for the updated functionalities and are correctly included.
526-526: PassingcoverAllImpactedProjectsappropriately.Including
coverAllImpactedProjectsin the call toConvertJobsToDiggerJobsensures that the batch creation process is aware of the coverage context.
831-831: Ensure consistent error handling with new return values.The function
generic.ConvertIssueCommentEventToJobsnow returnscoverAllImpactedProjects. Verify that any errors or exceptions related to this new return value are properly handled downstream to prevent unintended behavior.
934-934: Consistent usage ofcoverAllImpactedProjectsin batch processing.Passing
coverAllImpactedProjectsintoConvertJobsToDiggerJobsaligns with the updated logic, ensuring batch creation correctly accounts for all impacted projects.backend/controllers/projects_test.go (3)
4-6: Import statements consolidated.Duplicate imports of
net/httpandtestinghave been removed, improving code cleanliness.
72-77: Test setup enhanced with new fields.Added fields such as
GithubInstallationId,RepoFullName,RepoOwner,RepoName,BatchType, andCoverAllImpactedProjectsprovide a more comprehensive context for the tests.
103-113: Extended test coverage forCoverAllImpactedProjects.The new test case where
CoverAllImpactedProjectsis set tofalseeffectively verifies the behavior ofAutomergePRforBatchIfEnabledwhen not all impacted projects are covered, ensuring the merge action is appropriately controlled.backend/models/scheduler.go (1)
6-8: Reorganized import statements.Moved
logandtimeimports to the top, adhering to Go's import grouping conventions.backend/models/storage.go (1)
621-640: LGTM! Clean integration of coverage tracking.The
coverAllImpactedProjectsfield is properly integrated into the batch creation process, maintaining a clean and consistent structure.ee/backend/controllers/gitlab.go (3)
6-12: LGTM! Import statements are well-organized.The added standard library imports are necessary for the implemented functionality.
241-241: LGTM! Function call updated to support auto_merge fix.The addition of
coverAllImpactedProjectsparameter aligns with the PR objectives to fix auto_merge functionality in backend mode.
484-484: LGTM! Consistent implementation with pull request handling.The addition of
coverAllImpactedProjectsparameter maintains consistency with the pull request event handling changes.
| RepoName string | ||
| BatchType orchestrator_scheduler.DiggerCommand | ||
| ReportTerraformOutputs bool | ||
| CoverAllImpactedProjects bool |
There was a problem hiding this comment.
💡 Codebase verification
CoverAllImpactedProjects is not included in serialization
The MapToJsonStruct method in DiggerBatch does not include the CoverAllImpactedProjects field in its serialization output, which could lead to data loss. The field needs to be added to the SerializedBatch struct returned by the method.
🔗 Analysis chain
Verify serialization of CoverAllImpactedProjects.
Ensure that the new field CoverAllImpactedProjects in the DiggerBatch struct is correctly handled during serialization, especially in the MapToJsonStruct method, to prevent data inconsistencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for serialization of 'CoverAllImpactedProjects' in methods.
# Look for occurrences where DiggerBatch is converted without including 'CoverAllImpactedProjects'.
ast-grep --pattern $'func ($_ *DiggerBatch) MapToJsonStruct() {...}' --json | jq '.captures[] | select(.value | test("CoverAllImpactedProjects")) | .value'
Length of output: 224
Script:
#!/bin/bash
# Find the MapToJsonStruct method implementation and surrounding context
rg -U "func.*MapToJsonStruct.*DiggerBatch" -A 20
# Search for struct definition and JSON tags
rg "CoverAllImpactedProjects.*\`json:"
# Look for test files that might verify serialization
rg "TestDiggerBatch.*MapToJsonStruct" -l
Length of output: 133
Script:
#!/bin/bash
# Show the content of scheduler.go
cat backend/models/scheduler.go
# Find any JSON-related code in the models directory
rg "json" backend/models/ -A 5 -B 5
# Look for test files in the models directory
fd ".*_test.go" backend/models/
Length of output: 12644
duy0611
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/controllers/projects_test.go (1)
104-113: Consider extracting repeated config to a constant.While the test case correctly verifies auto-merge behavior when all projects are covered, the digger config string is duplicated multiple times in the test. Consider extracting it to a constant for better maintainability.
+ const autoMergeEnabledConfig = "" + + "projects:\n" + + " - name: dev\n" + + " dir: dev\n" + + "auto_merge: true" func TestAutomergeWhenBatchIsSuccessfulStatus(t *testing.T) { // ... existing code ... - batch.DiggerConfig = "" + - "projects:\n" + - " - name: dev\n" + - " dir: dev\n" + - "auto_merge: true" + batch.DiggerConfig = autoMergeEnabledConfig
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/controllers/projects_test.go(3 hunks)
🔇 Additional comments (3)
backend/controllers/projects_test.go (3)
4-6: Clean import organization looks good!The consolidation of import statements improves code organization.
99-102: Test case correctly verifies auto-merge prevention.Good test coverage for the scenario where not all impacted projects are covered, ensuring auto-merge is prevented.
72-77: Verify field initialization aligns with production code.The addition of
CoverAllImpactedProjectsand its initialization look good. This change aligns with the PR objectives to fix auto-merge behavior.Let's verify the field is consistently initialized in production code:
✅ Verification successful
Field initialization and usage verified successfully
The
CoverAllImpactedProjectsfield is consistently used in both production and test code:
- Used in auto-merge logic alongside other required conditions
- Test cases cover both true/false scenarios with appropriate assertions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for CoverAllImpactedProjects initialization patterns rg -A 3 "CoverAllImpactedProjects.*=.*" --type goLength of output: 1083
When running with Digger backend mode, PR is merged when
auto_mergeis enabled even though not all impacted projects have been applied. This is caused because Digger batch does not have context whether all the needed jobs have been triggered when usingdigger applywith-pflag.Unlike in backendless mode, where all the jobs are run sequentially and the parent one does have context if all the impacted projects have been applied, in backend mode jobs are triggered via the Batch one, which does not have this context. This PR addresses the issue by adding one more column to store this context, which will then be used in the subsequent run.
Summary by CodeRabbit
Release Notes: Project Coverage Enhancement
New Features
Database Changes
cover_all_impacted_projectscolumn in digger batches, enabling more granular control over project coverage during batch processing.Improvements