Skip to content

feat!: Add merge queue parameters to repository ruleset - #3253

Merged
gmlewis merged 4 commits into
google:masterfrom
zepeng811:issue_3225
Sep 3, 2024
Merged

feat!: Add merge queue parameters to repository ruleset#3253
gmlewis merged 4 commits into
google:masterfrom
zepeng811:issue_3225

Conversation

@zepeng811

@zepeng811zepeng811 commented Aug 28, 2024

Copy link
Copy Markdown
Contributor

Fixes: #3225.

BREAKING CHANGE: NewMergeQueueRule now takes one parameter: *MergeQueueRuleParameters.

Summary

closes: #3225
(I'm the same author as the assignee of the issue @zhpeng811, but submitting the
PR using my corporate account)

Details

This PR adds merge queue parameter configurations to the repository branch ruleset.

an attempt was also made for organization ruleset but was later determined that GitHub currently does not support merge queue configurations at the organizational level. Configs related to merge queue in tests was removed as the result.

Testing

General

Ran the required scripts on step 4 of the CONTRIBUTING.md file

Details

❯ script/fmt.sh
❯ script/test.sh
testing .
? github.com/google/go-github/v63/test/integration [no test files]
ok github.com/google/go-github/v63/github (cached) coverage: 99.4% of statements
github.com/google/go-github/v63/test/fields coverage: 0.0% of statements
testing example
github.com/google/go-github/v63/example/actionpermissions coverage: 0.0% of statements
github.com/google/go-github/v63/example/basicauth coverage: 0.0% of statements
github.com/google/go-github/v63/example/codespaces/newusersecretwithxcrypto coverage: 0.0% of statements
github.com/google/go-github/v63/example/codespaces/newreposecretwithxcrypto coverage: 0.0% of statements
github.com/google/go-github/v63/example/appengine coverage: 0.0% of statements
github.com/google/go-github/v63/example/newfilewithappauth coverage: 0.0% of statements
github.com/google/go-github/v63/example/listenvironments coverage: 0.0% of statements
github.com/google/go-github/v63/example/commitpr coverage: 0.0% of statements
github.com/google/go-github/v63/example/migrations coverage: 0.0% of statements
github.com/google/go-github/v63/example/newrepo coverage: 0.0% of statements
github.com/google/go-github/v63/example/newreposecretwithxcrypto coverage: 0.0% of statements
github.com/google/go-github/v63/example/simple coverage: 0.0% of statements
github.com/google/go-github/v63/example/ratelimit coverage: 0.0% of statements
github.com/google/go-github/v63/example/tagprotection coverage: 0.0% of statements
github.com/google/go-github/v63/example/tokenauth coverage: 0.0% of statements
github.com/google/go-github/v63/example/topics coverage: 0.0% of statements
testing scrape
ok github.com/google/go-github/scrape (cached) coverage: 59.4% of statements
github.com/google/go-github/scrape/example/scrape coverage: 0.0% of statements
testing tools
ok tools/metadata (cached) coverage: 81.7% of statements
❯ script/lint.sh
linting .
linting example
linting scrape
linting tools
validating generated files

Repository Ruleset

Manual:
Tested with the following test code, both with and without including the specific parameters, and verified in the UI.

Expand to view code

funcmain() {
client:=github.NewClient(nil).WithAuthToken(os.Getenv("GITHUB_TOKEN"))
org:="<REPLACE_WITH_YOUR_ORG>"repo:="<REPLACE_WITH_YOUR_REPO>"ctx:=context.Background()
_, _, err:=client.Repositories.CreateRuleset(ctx, org, repo, &github.Ruleset{
Name: "merge-queue-test-without-param",
Enforcement: "active",
Rules: []*github.RepositoryRule{
github.NewMergeQueueRule(nil),
},
})
iferr!=nil {
fmt.Println(err)
}
_, _, err=client.Repositories.CreateRuleset(ctx, org, repo, &github.Ruleset{
Name: "merge-queue-test-with-param",
Enforcement: "active",
Rules: []*github.RepositoryRule{
github.NewMergeQueueRule(&github.MergeQueueRuleParameters{
CheckResponseTimeoutMinutes: 35,
GroupingStrategy: "HEADGREEN",
MaxEntriesToBuild: 8,
MaxEntriesToMerge: 4,
MergeMethod: "SQUASH",
MinEntriesToMerge: 2,
MinEntriesToMergeWaitMinutes: 13,
}),
},
})
iferr!=nil {
fmt.Println(err)
}
}

Unit Test:
A unit test already exist for merge queue without parameters, 2 more tests were added (1 success case and 1 failure case with the incorrect parameter type)

Organization Ruleset

Was trying to create a org ruleset with merge queue rule using the following curl

Expand for curl

export ORG_NAME=<REPLACE_WITH_YOUR_ORG_NAME>
curl -L \
-X POST \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer $GITHUB_TOKEN" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/orgs/$ORG_NAME/rulesets \
-d '{"name":"merge queue test ruleset","target":"branch","enforcement":"active","conditions":{"ref_name":{"include":["refs/heads/main","refs/heads/master"],"exclude":["refs/heads/dev*"]},"repository_name":{"include":["test1","test2"],"exclude":["test3"],"protected":true}},"rules":[{"type":"merge_queue"}]}'

but got the following error:

{
"message": "Validation Failed",
"errors": [
"Invalid rules: 'Merge queue'"
],
"documentation_url": "https://docs.github.com/rest/orgs/rules#create-an-organization-repository-ruleset",
"status": "422"
}

I cross referenced with the UI page and did not notice a branch rule for merge queue, which conflicts with the REST API documentation for org rulesets, so seems like GitHub currently does not support merge queue configuration on the org level ruleset. Reported the issue to GitHub community: https://github.com/orgs/community/discussions/137097

@zepeng811
zepeng811 marked this pull request as draft August 28, 2024 21:16
@gmlewisgmlewis changed the title feat: add merge queue parameters to repository rulesetfeat!: Add merge queue parameters to repository rulesetAug 28, 2024
@gmlewisgmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Aug 28, 2024
@codecov

codecovBot commented Aug 28, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.94%. Comparing base (2b8c7fa) to head (02f3dc8).
Report is 105 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #3253 +/- ##
==========================================
- Coverage 97.72% 92.94% -4.78% 
==========================================
Files 153 171 +18 Lines 13390 11663 -1727 ==========================================
- Hits 13085 10840 -2245 - Misses 215 729 +514 - Partials 90 94 +4 

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment threadgithub/repos_rules.go
}

// MergeQueueRuleParameters represents the merge_queue rule parameters.
type MergeQueueRuleParameters struct {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zepeng811 - should any of these parameters be optional (and therefore pointers with the omitempty JSON tag added)?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmlewis did a quick test and seems like it must be all or nothing

adding all parameters like this worked:

curl -L \
-X POST \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer $GITHUB_TOKEN" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/$ORG/$REPO/rulesets \
-d '{"name":"merge queue test ruleset","target":"branch","enforcement":"active","conditions":{"ref_name":{"include":["refs/heads/main","refs/heads/master"],"exclude":["refs/heads/dev*"]}},"rules":[{"type":"merge_queue","parameters":{"check_response_timeout_minutes":35,"grouping_strategy":"HEADGREEN","max_entries_to_build":8,"max_entries_to_merge":4,"merge_method":"SQUASH","min_entries_to_merge":2,"min_entries_to_merge_wait_minutes":13}}]}'

but if any parameter was removed, like:

curl -L \
-X POST \
-H "Accept: application/vnd.github+json" \
-H "Authorization: Bearer $GITHUB_TOKEN" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/$ORG/$REPO/rulesets \
-d '{"name":"merge queue test ruleset 2","target":"branch","enforcement":"active","conditions":{"ref_name":{"include":["refs/heads/main","refs/heads/master"],"exclude":["refs/heads/dev*"]}},"rules":[{"type":"merge_queue","parameters":{"check_response_timeout_minutes":35,"grouping_strategy":"HEADGREEN","max_entries_to_build":8,"max_entries_to_merge":4,"merge_method":"SQUASH","min_entries_to_merge":2}}]}'

then the API will error:

{
"message": "Invalid request.\n\nInvalid property /rules/0: data matches no possible input. See `documentation_url`.",
"documentation_url": "https://docs.github.com/rest/repos/rules#create-a-repository-ruleset",
"status": "422"
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for testing!

@zepeng811
zepeng811 marked this pull request as ready for review August 28, 2024 21:23

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, @zepeng811 !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @ali-kafel and @tomleicircle !
Merging.

@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 3, 2024
@gmlewis
gmlewis merged commit 00caddc into google:masterSep 3, 2024
jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Fixes: google#3225.
BREAKING CHANGE: `NewMergeQueueRule` now takes one parameter: `*MergeQueueRuleParameters`.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Breaking API ChangePR will require a bump to the major version num in next release. Look here to see the change(s).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add support for merge queue parameters for organization and repository ruleset

4 participants

@zepeng811@gmlewis@tomleicircle@ali-kafel