Skip to content

Remove params from repo rulesets update rule - #2836

Closed
o-sama wants to merge 1 commit into
google:masterfrom
o-sama:fix-update-rule
Closed

Remove params from repo rulesets update rule#2836
o-sama wants to merge 1 commit into
google:masterfrom
o-sama:fix-update-rule

Conversation

@o-sama

@o-samao-sama commented Jul 21, 2023

Copy link
Copy Markdown
Contributor

I was testing out this functionality and noticed that I can send a request with params set for update rule which is fine since it seems the GitHub API doesn’t actually check parameters with this rule as it doesn’t expect any.

However, if I try reading a ruleset which contains an update rule, it returns an object with ”type”: “update" and no parameters. This is also the same behaviour as the GitHub UI where you cannot set parameters for update rules or see any parameters for it. I looked at the request to read rulesets as sent by the frontend and found:

{
"type": "update",
"target": "branch",
"displayName": "Restrict updates",
"description": "Only allow users with bypass permission to update matching refs.",
"parameterSchema": {
"type": "object",
"name": null,
"display_name": null,
"description": null,
"required": true,
"root": true,
"internal": false,
"org_only": false,
"supported_plan": null,
"default_value": null,
"validator": null,
"ui_control": null,
"visibility_fn": null,
"fields": []
},
"metadataPatternSchema": null
}

Leads me to think there was an undocumented change in behaviour in the API.

@codecov

codecovBot commented Jul 21, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2836 (180ce52) into master (b02bb75) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2836 +/- ##
==========================================
- Coverage 98.06% 98.06% -0.01% 
==========================================
Files 137 137 Lines 12291 12277 -14 ==========================================
- Hits 12053 12039 -14 
Misses 162 162 Partials 76 76 
Impacted FilesCoverage Δ
github/repos_rules.go100.00% <100.00%> (ø)

@gmlewisgmlewis changed the title remove params from repo rulesets update ruleRemove params from repo rulesets update ruleJul 21, 2023
@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 Jul 21, 2023
@gmlewis

Copy link
Copy Markdown
Collaborator

@liaodaniel - can you please take a look since you are the original author?

@liaodaniel

Copy link
Copy Markdown
Contributor

@o-sama, you are indeed correct. I was able to reproduce this behavior as well.

The GitHub documentation still shows the following:
Screenshot 2023-07-21 at 11 36 47 pm

I wonder if it might be worth reaching out to the GitHub support team to confirm if this is the intended API behavior?

@gmlewis

Copy link
Copy Markdown
Collaborator

Thank you, @liaodaniel !

I wonder if it might be worth reaching out to the GitHub support team to confirm if this is the intended API behavior?

That would be great if one of you would like to volunteer. 😁💜

@o-sama

Copy link
Copy Markdown
ContributorAuthor

I’m actually meeting with some folks from GitHub in a couple of hours for something else, I’ll bring this up and confirm 😄

@gmlewis

Copy link
Copy Markdown
Collaborator

I’m actually meeting with some folks from GitHub in a couple of hours for something else, I’ll bring this up and confirm 😄

Thank you, @o-sama ! I appreciate it!

@o-sama

Copy link
Copy Markdown
ContributorAuthor

Question is being forwarded to the PM responsible for repo rules to confirm whether it is an upcoming feature or an older feature which was cut. Will update here and if it is an upcoming feature I’ll close out PR 🙂

@felixlut

Copy link
Copy Markdown
Contributor

Could I ask for a new release once this issue is resolved? I'm working on implementing Repository Rulesets in the github terraform provider, which is currently blocked by this

@o-sama

Copy link
Copy Markdown
ContributorAuthor

@felixlut I’ve actually also already started working on repo/org rulesets in the TF provider and have all CRUD working for repo rulesets, with the focus shifting to org rulesets in the next couple of days. I just made a WIP PR and mentioned you in it, If you’d like we can pair on it?

@felixlut

Copy link
Copy Markdown
Contributor

@o-sama Sure, I'll take a proper look at your code tomorrow after work, and we can combine the best parts! As you can see in my PR, I very much wanted feedback anyways 😄

@o-sama

Copy link
Copy Markdown
ContributorAuthor

JFYI: I haven’t forgotten about this PR, just waiting on feedback on the question I raised to my contact at GitHub who comes back from vacation on Friday.

@o-sama

Copy link
Copy Markdown
ContributorAuthor

Looks like this is a feature available specifically for forked repos. However, I was told it’s bugged even for those. Closing this since it’s unneeded.

@o-samao-sama closed this Aug 1, 2023
@gmlewisgmlewis removed the NeedsReview PR is awaiting a review before merging. label May 5, 2024
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.

4 participants

@o-sama@gmlewis@liaodaniel@felixlut