support auto_merge_strategy option #1922
Conversation
WalkthroughThis pull request updates the pull request merging functionality by introducing a new parameter, Changes
Sequence Diagram(s)sequenceDiagram
participant Controller as Projects Controller
participant Converter as Config Converter
participant Validator as Strategy Validator
participant CI as CI Service
Controller->>Converter: Load YAML config
Converter->>Validator: Validate 'auto_merge_strategy'
Validator-->>Converter: Validated strategy (or error)
Converter-->>Controller: Return DiggerConfig with AutoMergeStrategy
Controller->>CI: MergePullRequest(prNumber, auto_merge_strategy)
CI-->>Controller: Return merge result/error
Possibly related PRs
Poem
✨ Finishing Touches
🪧 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
This PR adds configurable merge strategies for auto-merging pull requests in Digger, with significant changes across the codebase.
- Critical bug in
ValidateAutomergeStrategyerror message showingAutomergeStrategyRebasetwice instead ofAutomergeStrategyMerge - Merge strategy parameter not implemented in Bitbucket, GitLab, and Azure integrations despite interface changes
- Incorrect condition in Azure's
GetCombinedPullRequestStatusfunction using OR instead of AND for success check - Documentation should clarify that merge strategies are GitHub-only and explain the default behavior for other VCS providers
19 file(s) reviewed, 7 comment(s)
Edit PR Review Bot Settings | Greptile
| func (m *MockPRManager) MergePullRequest(prNumber int, mergeStrategy string) error { | ||
| m.Commands = append(m.Commands, RunInfo{"MergePullRequest", strconv.Itoa(prNumber), time.Now()}) | ||
| return nil | ||
| } |
There was a problem hiding this comment.
logic: The mergeStrategy parameter is not being logged in the RunInfo struct, which means tests won't be able to verify which merge strategy was used. Consider including it in the Params field.
| If you are using github VCS you can also specify | ||
| `auto_merge_strategy` to define the type of strategy. Possible values are: |
There was a problem hiding this comment.
style: Consider clarifying that this is a GitHub-specific feature by starting with 'When using GitHub as your VCS' instead of 'If you are using github VCS'
| @@ -381,7 +381,7 @@ func (b BitbucketAPI) GetCombinedPullRequestStatus(prNumber int) (string, error) | |||
|
|
|||
| } | |||
|
|
|||
| func (b BitbucketAPI) MergePullRequest(prNumber int) error { | |||
| func (b BitbucketAPI) MergePullRequest(prNumber int, mergeStrategy string) error { | |||
| url := fmt.Sprintf("%s/repositories/%s/%s/pullrequests/%d/merge", bitbucketBaseURL, b.RepoWorkspace, b.RepoName, prNumber) | |||
|
|
|||
| resp, err := b.sendRequest("POST", url, nil) | |||
There was a problem hiding this comment.
syntax: ```suggestion
mergeBody := map[string]interface{}{
"merge_strategy": mergeStrategy,
}
mergeJSON, err := json.Marshal(mergeBody)
if err != nil {
return err
}
resp, err := b.sendRequest("POST", url, mergeJSON)
| _, _, err = svc.Client.PullRequests.Merge(context.Background(), svc.Owner, svc.RepoName, prNumber, "auto-merge", &github.PullRequestOptions{ | ||
| MergeMethod: "squash", | ||
| MergeMethod: mergeStrategy, | ||
| SHA: pr.Head.GetSHA(), | ||
| }) |
There was a problem hiding this comment.
style: Consider adding error handling for invalid merge strategies that GitHub API might reject.
| _, _, err = svc.Client.PullRequests.Merge(context.Background(), svc.Owner, svc.RepoName, prNumber, "auto-merge", &github.PullRequestOptions{ | |
| MergeMethod: "squash", | |
| MergeMethod: mergeStrategy, | |
| SHA: pr.Head.GetSHA(), | |
| }) | |
| validStrategies := map[string]bool{"merge": true, "squash": true, "rebase": true} | |
| if !validStrategies[mergeStrategy] { | |
| return fmt.Errorf("invalid merge strategy '%s'. Must be one of: merge, squash, rebase", mergeStrategy) | |
| } | |
| _, _, err = svc.Client.PullRequests.Merge(context.Background(), svc.Owner, svc.RepoName, prNumber, "auto-merge", &github.PullRequestOptions{ | |
| MergeMethod: mergeStrategy, | |
| SHA: pr.Head.GetSHA(), | |
| }) |
| func (t MockCiService) MergePullRequest(prNumber int, mergeStrategy string) error { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
style: The mock implementation returns nil without using the mergeStrategy parameter. Consider adding validation or tracking of the merge strategy to ensure tests can verify the correct strategy is being used.
libs/digger_config/config.go
Outdated
| @@ -11,6 +17,7 @@ type DiggerConfig struct { | |||
| PrLocks bool | |||
| Projects []Project | |||
| AutoMerge bool | |||
| AutoMergeStrategy string | |||
There was a problem hiding this comment.
logic: AutoMergeStrategy should be of type AutomergeStrategy (the custom type) rather than string to ensure type safety and validation
| AutoMergeStrategy string | |
| AutoMergeStrategy AutomergeStrategy |
libs/digger_config/validators.go
Outdated
| case string(AutomergeStrategySquash), string(AutomergeStrategyMerge), string(AutomergeStrategyRebase): | ||
| return nil | ||
| default: | ||
| return fmt.Errorf("invalid merge strategy: %v, valid values are: %v, %v, %v", strategy, AutomergeStrategySquash, AutomergeStrategyRebase, AutomergeStrategyRebase) |
There was a problem hiding this comment.
logic: Error message incorrectly lists AutomergeStrategyRebase twice instead of including AutomergeStrategyMerge. Should be: AutomergeStrategySquash, AutomergeStrategyMerge, AutomergeStrategyRebase
| return fmt.Errorf("invalid merge strategy: %v, valid values are: %v, %v, %v", strategy, AutomergeStrategySquash, AutomergeStrategyRebase, AutomergeStrategyRebase) | |
| return fmt.Errorf("invalid merge strategy: %v, valid values are: %v, %v, %v", strategy, AutomergeStrategySquash, AutomergeStrategyMerge, AutomergeStrategyRebase) |
![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
🔭 Outside diff range comments (2)
libs/ci/azure/azure.go (1)
276-294: 💡 Verification agent🧩 Analysis chain
Implementation doesn't use the new
mergeStrategyparameterThe method signature has been updated to include the
mergeStrategyparameter, but the implementation doesn't make use of it. While this maintains backward compatibility, the parameter is being passed but not utilized, which could be confusing.If different merge strategies need to be supported for Azure DevOps, consider implementing strategy-specific logic similar to how it's handled in other CI providers.
Let's verify if Azure DevOps API supports different merge strategies:
🌐 Web query:
What merge strategies does Azure DevOps API support for pull requests?💡 Result:
Azure DevOps API supports four merge strategies for pull requests, which determine how changes are integrated into the target branch. These strategies align with Git's merging capabilities and are accessible via the REST API's
mergeStrategyparameter:
Merge (no fast-forward)
- API value:
noFastForward- Preserves the full commit history of both branches, creating a merge commit that records the integration. This maintains a non-linear history but can clutter the default branch.
Squash commit
- API value:
squash- Combines all commits from the source branch into a single commit on the target branch. This simplifies the history but discoses granular commit details from the topic branch[2][5].
Rebase and fast-forward
- API value:
rebase- Rebases the source branch commits onto the target branch and fast-forwards the target branch pointer. This creates a linear history but rewrites commit hashes[2][10].
Semi-linear merge
- API value:
rebaseMerge- Rebases the source branch onto the target branch first, then performs a non-fast-forward merge. This preserves individual commits while explicitly documenting the merge via a dedicated commit[5][10].
Key API Implementation Details
- The merge strategy is specified in the
GitPullRequestMergeOptionsobject when completing a pull request via endpoints likePull Requests - CreateorPull Requests - Update[6][9].- Branch policies can enforce specific strategies (e.g., requiring squash merges) via the
Limit merge typessetting[3][10].- Multiple merge bases (divergent histories) may trigger warnings during API operations, requiring manual resolution before merging[1][3].
These strategies provide flexibility in balancing historical fidelity and repository cleanliness, with the API mirroring options available in the Azure DevOps UI.
Citations:
- 1: https://learn.microsoft.com/en-us/azure/devops/repos/git/merging-with-squash?view=azure-devops
- 2: https://learn.microsoft.com/en-us/azure/devops/repos/git/complete-pull-requests?view=azure-devops
- 3: https://learn.microsoft.com/en-us/azure/devops/repos/git/about-pull-requests?view=azure-devops
- 4: https://learn.microsoft.com/en-us/azure/devops/repos/git/pull-requests?view=azure-devops
- 5: https://anvay.hashnode.dev/merge-types-in-azure-repos
- 6: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-requests/get-pull-requests-by-project?view=azure-devops-rest-7.1
- 7: https://microsoft.github.io/code-with-engineering-playbook/source-control/merge-strategies/
- 8: https://www.youtube.com/watch?v=RXQ9jZmKzfE
- 9: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-requests/create?view=azure-devops-rest-7.1
- 10: https://dfe-analytical-services.github.io/azure-devops-for-analysis/devops---the-basics.html
- 11: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/merges/create?view=azure-devops-rest-7.1
- 12: https://developercommunity.visualstudio.com/t/policy-to-set-the-default-merge-type-in-the-comple/643703
- 13: https://learn.microsoft.com/en-us/rest/api/azure/devops/git/pull-requests/get-pull-requests?view=azure-devops-rest-7.1
- 14: https://developercommunity.visualstudio.com/idea/1332187/branch-policy-limiting-merge-types-from-a-given-br.html
- 15: https://developercommunity.visualstudio.com/t/REST-API-%E2%80%93-Incorrect-PullRequestAsyncSta/10576242?q=rest
Attention: Update MergePullRequest to Handle mergeStrategy
The function signature now accepts a
mergeStrategyparameter, but it isn’t used in the current implementation. Since Azure DevOps supports multiple merge strategies (e.g.,noFastForward,squash,rebase,rebaseMerge), it’s advisable to map and forward the providedmergeStrategyto the corresponding API parameter—likely via a merge options field—so that different merge behaviors can be explicitly supported.
- Map the input
mergeStrategyto Azure DevOps' supported API values.- Update the call to
a.Client.UpdatePullRequestto include merge strategy information.- Document the default merge behavior when no strategy is specified.
libs/ci/gitlab/gitlab.go (1)
218-232: 💡 Verification agent🧩 Analysis chain
Implementation doesn't utilize the new
mergeStrategyparameterThe method signature has been updated to accept the
mergeStrategyparameter, but the implementation doesn't apply this parameter when merging the pull request. GitLab supports different merge strategies that could be used here.Consider using the
mergeStrategyparameter to set the appropriate merge method in the GitLab API call, similar to:mergeWhenPipelineSucceeds := true -opt := &go_gitlab.AcceptMergeRequestOptions{MergeWhenPipelineSucceeds: &mergeWhenPipelineSucceeds} +opt := &go_gitlab.AcceptMergeRequestOptions{ + MergeWhenPipelineSucceeds: &mergeWhenPipelineSucceeds, + Squash: getBoolPointer(mergeStrategy == "squash"), + SquashCommitMessage: getMessagePointer(mergeStrategy == "squash", "Squashed commit"), +}Let's check what merge strategies GitLab API supports:
🌐 Web query:
What merge strategies does GitLab API support when merging merge requests?💡 Result:
GitLab's API supports three primary merge strategies when merging merge requests, determined by the project's configured merge method and optional parameters in the API call:
Merge Strategies
Merge Commit
Creates a merge commit even if fast-forwarding is possible, equivalent togit merge --no-ff. Configured viamerge_method: "merge"at the project level. The API always creates a merge commit unless using fast-forward strategies[1][6].Merge Commit with Semi-Linear History
Requires a linear commit history by rebasing before merging. Configured viamerge_method: "rebase_merge"(labeled as semi-linear in UI). Requires source branch rebasing if target branch has updates[1][6].Fast-Forward Merge
Merges without creating a merge commit when possible (git merge --ff). Configured viamerge_method: "ff". Fails if not fast-forwardable unless rebased first[1][6].Key API Parameters
While the merge method is set at the project level, these API parameters modify merge behavior:
PUT /projects/{id}/merge_requests/{iid}/merge
Parameter Effect squash: trueSquashes all commits into one before merging[1][4] merge_when_pipeline_succeeds: trueAuto-merges after successful CI/CD pipeline[5][7] shaEnsures HEAD matches before merging[4][7] Project merge methods are configured via:
PUT /projects/{id} -d merge_method=ff|merge|rebase_mergeExample Workflow
Create a merge request with squash enabled:
curl --request POST --header "PRIVATE-TOKEN: <your_token>" \ --data "source_branch=feature&target_branch=main&title=MyMR&squash=true" \ "https://gitlab.example.com/api/v4/projects/1/merge_requests"Merge using semi-linear strategy (requires project configured with
rebase_merge):curl --request PUT --header "PRIVATE-TOKEN: <your_token>" \ "https://gitlab.example.com/api/v4/projects/1/merge_requests/42/merge" \ --data "merge_when_pipeline_succeeds=true"The API enforces the project's merge method rules while allowing runtime control over squash and pipeline-based merging[1][4][7]. For merge trains or advanced strategies, GraphQL mutations are required[9].
Citations:
- 1: https://docs.gitlab.com/user/project/merge_requests/methods/
- 2: https://gist.github.com/joshjohanning/2537ebe8dad6875a9c33b4f24b35b2e7
- 3: https://forge.etsi.org/rep/help/api/merge_requests.md
- 4: https://docs.gitlab.com/api/merge_requests/
- 5: https://python-gitlab.readthedocs.io/en/stable/gl_objects/merge_requests.html
- 6: https://docs.gitlab.com/user/project/merge_requests/
- 7: https://docs.gitlab.com/api/merge_request_approvals/
- 8: https://docs.gitlab.com/api/projects/
- 9: https://gitlab.com/gitlab-org/gitlab/-/issues/354739
Action: Update MergePullRequest to apply mergeStrategy when merging
The current implementation ignores the new
mergeStrategyparameter. As verified, GitLab's API supports various merge approaches (merge commit, rebase, and fast-forward) with additional control via flags likesquash. If a squash merge is desired, update the options accordingly. For example:mergeWhenPipelineSucceeds := true -opt := &go_gitlab.AcceptMergeRequestOptions{MergeWhenPipelineSucceeds: &mergeWhenPipelineSucceeds} +opt := &go_gitlab.AcceptMergeRequestOptions{ + MergeWhenPipelineSucceeds: &mergeWhenPipelineSucceeds, + Squash: getBoolPointer(mergeStrategy == "squash"), + SquashCommitMessage: getMessagePointer(mergeStrategy == "squash", "Squashed commit"), +}Please consider if additional strategies should be explicitly handled—although project-level settings typically govern merge commit, rebase, or fast-forward behaviors. This change will ensure that the provided
mergeStrategyparameter is actively used in the merge process.
🧹 Nitpick comments (7)
docs/ce/howto/auto-merge.mdx (1)
17-22: Fix minor grammatical issues in the documentation.There are a couple of small issues in this paragraph:
- Missing comma after "github" in the first line
- Repetition of "merge" in the last bullet point making it slightly confusing
-If you are using github VCS you can also specify +If you are using github, VCS you can also specify -merge: for merge commits merge +merge: for merge commits🧰 Tools
🪛 LanguageTool
[uncategorized] ~17-~17: Possible missing comma found.
Context: ... on github ``` If you are using github VCS you can also specify `auto_merge_strate...(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~21-~21: Possible typo: you repeated a word.
Context: ...: for squash merge - rebase: for rebase merge - merge: for merge commits merge(ENGLISH_WORD_REPEAT_RULE)
libs/ci/bitbucket/bitbucket.go (1)
384-398: The mergeStrategy parameter is not utilized in the implementation.While the method signature has been updated to include the
mergeStrategyparameter, the implementation doesn't make use of it when making the API request to Bitbucket. If Bitbucket supports different merge strategies, consider updating the implementation to include this parameter in the request.func (b BitbucketAPI) MergePullRequest(prNumber int, mergeStrategy string) error { url := fmt.Sprintf("%s/repositories/%s/%s/pullrequests/%d/merge", bitbucketBaseURL, b.RepoWorkspace, b.RepoName, prNumber) - resp, err := b.sendRequest("POST", url, nil) + // Create a request body that includes the merge strategy if provided + var requestBody []byte + if mergeStrategy != "" { + body := map[string]string{"merge_strategy": mergeStrategy} + var err error + requestBody, err = json.Marshal(body) + if err != nil { + return fmt.Errorf("failed to marshal merge strategy: %v", err) + } + } + + resp, err := b.sendRequest("POST", url, requestBody) if err != nil { return err }You may need to adjust this implementation based on the specific merge strategy options supported by the Bitbucket API.
cli/pkg/digger/digger_test.go (1)
116-118: Consider updating the mock to use the newmergeStrategyparameterThe
MockPRManagerimplementation has been updated to include themergeStrategyparameter to match the interface change, but it doesn't actually use this parameter in the implementation. Consider updating theCommandsappend operation to include the merge strategy in the parameters string.-func (m *MockPRManager) MergePullRequest(prNumber int, mergeStrategy string) error { - m.Commands = append(m.Commands, RunInfo{"MergePullRequest", strconv.Itoa(prNumber), time.Now()}) +func (m *MockPRManager) MergePullRequest(prNumber int, mergeStrategy string) error { + m.Commands = append(m.Commands, RunInfo{"MergePullRequest", strconv.Itoa(prNumber) + " " + mergeStrategy, time.Now()})This would help ensure that tests properly validate that the correct merge strategy is being passed through the call chain.
libs/digger_config/config.go (1)
20-20: Add a default value for AutoMergeStrategy in the defaultWorkflow function.While you've added the field to the struct, it's not initialized in the
defaultWorkflow()function. Consider adding it there to ensure consistent defaults throughout the application.func defaultWorkflow() *Workflow { return &Workflow{ Configuration: &WorkflowConfiguration{ OnCommitToDefault: []string{"digger unlock"}, OnPullRequestPushed: []string{"digger plan"}, OnPullRequestConvertedToDraft: []string{}, OnPullRequestClosed: []string{"digger unlock"}, SkipMergeCheck: false, }, + // Other fields... } }cli/pkg/digger/digger.go (1)
794-831: Consider adding documentation for the new parameter.While the code change is straightforward, adding a comment to explain the purpose and expected values of the
mergeStrategyparameter would improve code clarity for future maintainers.+// MergePullRequest attempts to merge a pull request using the specified merge strategy. +// Valid merge strategies include "squash", "merge", and "rebase" as defined in AutomergeStrategy constants. func MergePullRequest(ciService ci.PullRequestService, prNumber int, mergeStrategy string) {backend/controllers/projects.go (2)
741-741: Simplify boolean comparison.The comparison with a boolean constant is unnecessary and can be simplified.
- if batch.Status == orchestrator_scheduler.BatchJobSucceeded && batch.BatchType == orchestrator_scheduler.DiggerCommandApply && automerge == true { + if batch.Status == orchestrator_scheduler.BatchJobSucceeded && batch.BatchType == orchestrator_scheduler.DiggerCommandApply && automerge {🧰 Tools
🪛 golangci-lint (1.64.8)
741-741: S1002: should omit comparison to bool constant, can be simplified to
automerge(gosimple)
725-754: Consider validating the merge strategy.While the validation is likely happening in the converter function, consider adding a check here to ensure the strategy is valid before attempting to use it. This would provide an additional safeguard.
automerge := config.AutoMerge automergeStrategy := config.AutoMergeStrategy + // Ensure we have a valid strategy value + if automergeStrategy == "" { + automergeStrategy = string(digger_config.AutomergeStrategySquash) // Default to squash if not specified + } if batch.Status == orchestrator_scheduler.BatchJobSucceeded && batch.BatchType == orchestrator_scheduler.DiggerCommandApply && automerge {🧰 Tools
🪛 golangci-lint (1.64.8)
741-741: S1002: should omit comparison to bool constant, can be simplified to
automerge(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/controllers/projects.go(1 hunks)cli/pkg/digger/digger.go(2 hunks)cli/pkg/digger/digger_test.go(1 hunks)cli/pkg/github/github.go(3 hunks)docs/ce/howto/auto-merge.mdx(1 hunks)docs/ce/reference/digger.yml.mdx(1 hunks)libs/ci/azure/azure.go(1 hunks)libs/ci/bitbucket/bitbucket.go(1 hunks)libs/ci/ci.go(1 hunks)libs/ci/github/github.go(2 hunks)libs/ci/github/mocks.go(1 hunks)libs/ci/gitlab/gitlab.go(1 hunks)libs/ci/mocks.go(1 hunks)libs/comment_utils/reporting/reporting_test.go(1 hunks)libs/digger_config/config.go(2 hunks)libs/digger_config/converters.go(1 hunks)libs/digger_config/validators.go(1 hunks)libs/digger_config/yaml.go(1 hunks)libs/orchestrator/mock.go(1 hunks)
🧰 Additional context used
🧬 Code Definitions (15)
libs/ci/github/github.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/comment_utils/reporting/reporting_test.go (2)
libs/ci/github/mocks.go (1)
MockCiService(9-11)cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/digger_config/validators.go (1)
libs/digger_config/config.go (3)
AutomergeStrategySquash(8-8)AutomergeStrategyMerge(9-9)AutomergeStrategyRebase(10-10)
libs/orchestrator/mock.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/ci/mocks.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/digger_config/converters.go (2)
libs/digger_config/validators.go (1)
ValidateAutomergeStrategy(5-12)libs/digger_config/config.go (1)
AutomergeStrategySquash(8-8)
libs/ci/ci.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/ci/gitlab/gitlab.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
cli/pkg/github/github.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/ci/azure/azure.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/ci/bitbucket/bitbucket.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
cli/pkg/digger/digger_test.go (1)
cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
cli/pkg/digger/digger.go (1)
libs/ci/ci.go (1)
PullRequestService(5-27)
backend/controllers/projects.go (2)
libs/digger_config/converters.go (1)
ConvertDiggerYamlToConfig(185-325)cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
libs/ci/github/mocks.go (2)
libs/comment_utils/reporting/reporting_test.go (1)
MockCiService(9-11)cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
🪛 LanguageTool
docs/ce/howto/auto-merge.mdx
[uncategorized] ~17-~17: Possible missing comma found.
Context: ... on github ``` If you are using github VCS you can also specify `auto_merge_strate...
(AI_HYDRA_LEO_MISSING_COMMA)
[duplication] ~21-~21: Possible typo: you repeated a word.
Context: ...: for squash merge - rebase: for rebase merge - merge: for merge commits merge
(ENGLISH_WORD_REPEAT_RULE)
🪛 golangci-lint (1.64.8)
backend/controllers/projects.go
741-741: S1002: should omit comparison to bool constant, can be simplified to automerge
(gosimple)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (18)
libs/digger_config/yaml.go (1)
16-16: LGTM! Good addition to the config model.The new
AutoMergeStrategyfield properly follows the established patterns in this struct: using a pointer type for an optional value and following the naming conventions (camelCase for the field name, snake_case for the YAML tag).docs/ce/howto/auto-merge.mdx (1)
14-14: LGTM! Clear documentation of the new feature.The new configuration option is well-documented with a helpful comment indicating it's optional and GitHub-specific.
libs/digger_config/converters.go (1)
212-220: LGTM! Well-implemented validation with default value.The implementation follows the same pattern as other configuration options in this file, properly validating the input and setting a sensible default when the value is not provided.
libs/ci/mocks.go (1)
46-48: Method signature correctly updated to include merge strategy parameter.The signature for
MergePullRequesthas been updated to include the newmergeStrategyparameter, which aligns with the changes made to the interface in other parts of the codebase.libs/ci/github/github.go (1)
287-321: Implementation correctly updated to use the mergeStrategy parameter.The
MergePullRequestmethod signature was updated to include themergeStrategyparameter, and this value is now being used in the GitHub API call on line 317 instead of a hardcoded merge method. This change properly enables configurable merge strategies.docs/ce/reference/digger.yml.mdx (1)
101-110: Documentation for the new auto_merge_strategy option is clear and comprehensive.The new
auto_merge_strategyconfiguration option is well documented with appropriate type, default value, and a clear description of all possible values. The note about GitHub-only support provides important context for users.libs/orchestrator/mock.go (1)
50-53: Method signature correctly updated for the mock implementation.The mock implementation of
MergePullRequesthas been properly updated to include the newmergeStrategyparameter, keeping it consistent with the updated interface. The command tracking behavior remains unchanged.libs/ci/github/mocks.go (1)
62-63: Method signature updated correctly to match interface.The
MergePullRequestmethod inMockCiServicehas been updated to include the newmergeStrategyparameter, aligning with the updated interface definition inlibs/ci/ci.go. The mock implementation appropriately returnsnilwithout using the parameter, which is typical for test mocks.libs/ci/ci.go (1)
18-18: Interface contract modified to support merge strategy customization.The
PullRequestServiceinterface has been updated to include amergeStrategyparameter in theMergePullRequestmethod, which enables clients to specify how pull requests should be merged. This change is consistent with the modifications in the implementations across the codebase.libs/comment_utils/reporting/reporting_test.go (1)
62-64: Method signature updated correctly to match the interface.The
MergePullRequestmethod in this test mock has been updated to include the newmergeStrategyparameter, aligning with changes to thePullRequestServiceinterface. This ensures the mock implementation remains compatible with the interface contract.cli/pkg/github/github.go (3)
305-305: Correctly passes the auto merge strategy parameterThe implementation correctly passes the
diggerConfig.AutoMergeStrategyparameter to theMergePullRequestfunction, which will allow for configurable merge strategies.
179-186: Code formatting changes improve readabilityThe formatting changes to these lines improve code readability by making variable assignments consistent and clear.
207-208: Code formatting improvements for role ARN assignmentsThe changes to these lines improve the consistency of the variable naming and assignment pattern throughout the codebase.
libs/digger_config/config.go (1)
6-10: Good addition of typed constants for merge strategies.Adding a dedicated type with constants for the automerge strategies provides type safety and helps ensure that only valid values are used throughout the codebase.
cli/pkg/digger/digger.go (2)
794-794: Function signature updated to support merge strategy selection.The function now accepts an additional parameter for the merge strategy, aligning with the interface defined in the
libs/ci/ci.gofile. This provides flexibility in how PRs are merged.
824-824: Parameter correctly passed to the CI service.The merge strategy is now being passed to the CI service's MergePullRequest method, completing the implementation of this feature.
backend/controllers/projects.go (2)
732-740: Good error handling and config extraction.The code correctly loads the configuration and extracts both the automerge flag and the new strategy setting. Error handling has been improved to provide more specific error messages.
747-747: Correctly passing the merge strategy parameter.The code now passes the configured automerge strategy to the PR service, implementing the feature end-to-end.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/controllers/projects.go (1)
741-741: Simplify boolean comparison.The comparison with
trueis unnecessary and can be simplified.- if batch.Status == orchestrator_scheduler.BatchJobSucceeded && batch.BatchType == orchestrator_scheduler.DiggerCommandApply && automerge == true { + if batch.Status == orchestrator_scheduler.BatchJobSucceeded && batch.BatchType == orchestrator_scheduler.DiggerCommandApply && automerge {🧰 Tools
🪛 golangci-lint (1.64.8)
741-741: S1002: should omit comparison to bool constant, can be simplified to
automerge(gosimple)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/controllers/projects.go(1 hunks)cli/pkg/digger/digger.go(2 hunks)libs/digger_config/config.go(2 hunks)libs/digger_config/converters.go(1 hunks)libs/digger_config/validators.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- libs/digger_config/validators.go
- libs/digger_config/converters.go
- libs/digger_config/config.go
🧰 Additional context used
🧬 Code Definitions (2)
cli/pkg/digger/digger.go (2)
libs/ci/ci.go (1)
PullRequestService(5-27)libs/digger_config/config.go (1)
AutomergeStrategy(6-6)
backend/controllers/projects.go (2)
libs/digger_config/converters.go (1)
ConvertDiggerYamlToConfig(185-325)cli/pkg/digger/digger.go (1)
MergePullRequest(794-831)
🪛 golangci-lint (1.64.8)
backend/controllers/projects.go
741-741: S1002: should omit comparison to bool constant, can be simplified to automerge
(gosimple)
🔇 Additional comments (4)
cli/pkg/digger/digger.go (1)
794-831: Approve the PR's implementation adding merge strategy support.This change adds the
mergeStrategyparameter to theMergePullRequestfunction and correctly passes it to the CI service. This enhances the functionality by allowing users to specify how they want pull requests to be merged.backend/controllers/projects.go (3)
732-736: Good addition of explicit error handling for config loading.This improved error handling makes it easier to debug issues by providing more specific error messages when the configuration can't be loaded from YAML.
738-740: Clean implementation of the merge strategy configuration.The code now properly retrieves both the
AutoMergeflag and theAutoMergeStrategyfrom the configuration object, which is cleaner than checking for the presence of fields directly.
747-747: Approve the updated PR merge call with strategy parameter.The code now correctly passes the
automergeStrategyto the PR service's merge function, aligning with the changes in theMergePullRequestfunction.
Summary by CodeRabbit
New Features
Documentation