Allow an empty array of BypassActors in Ruleset struct in CreateRuleset endpoint #3174

Matthew-Reidy · 2024-05-22T18:56:17Z

added an extra Ruleset struct and CreateRuleset endpoint in repo_rules.go to allow the BypassActors field to be passed as nil or an empty array

…in repo_rules.go

Co-authored-by: Daniel Liao <[email protected]>

codecov · 2024-05-22T19:00:16Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (2b8c7fa) to head (fa5bba3).
Report is 78 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3174      +/-   ##
==========================================
- Coverage   97.72%   92.92%   -4.80%     
==========================================
  Files         153      171      +18     
  Lines       13390    11582    -1808     
==========================================
- Hits        13085    10763    -2322     
- Misses        215      726     +511     
- Partials       90       93       +3

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

gmlewis

It think it might be a more pleasant user experience to not introduce a new struct,
but just a new endpoint that still uses the Ruleset struct as input, but internally uses a non-exported struct like the one you created.

To do this, you would need to copy over the fields to this non-exported struct and then send that instead of the original Ruleset.

Also, let's please not start using BPActr as a short abbreviation for BypassActor, as we try to make this repo as searchable as possible, and nobody is going to guess bpactr as a search string.

Also, all new godoc-style comments should be complete sentences with ending periods. Thanks.

Matthew-Reidy · 2024-05-23T16:39:30Z

Do you mean something like having an anonymous struct identical to Ruleset in the new functions scope that would just copy over the values of Ruleset to? I understand your idea but i'm confused about the implementation.

gmlewis · 2024-05-23T18:08:01Z

Do you mean something like having an anonymous struct identical to Ruleset in the new functions scope that would just copy over the values of Ruleset to? I understand your idea but i'm confused about the implementation.

Yes, that is what I mean.

We have other examples of this in our repo.

Here is one of many: https://github.com/google/go-github/blob/master/github/repos.go#L546-L573

I found this by typing:

$ grep 'type [a-z]' github/*.go

Matthew-Reidy · 2024-05-30T19:25:02Z

made some changes. Let me know if its what you expected.

gmlewis

Yes, this is what I was talking about - the only difference is that the new struct should not be exported because the end-user never has know or worry about it.

github/repos_rules.go

Co-authored-by: Glenn Lewis <[email protected]>

Matthew-Reidy · 2024-05-30T20:00:38Z

Ok. I commited your suggested changes. I didn't know rulesetNoOmitBypassActors made it non exported. Learned something new about the language. Thanks!

gmlewis

Whups, sorry... my bad... I gave bad suggestions. Doh.

github/repos_rules.go

Co-authored-by: Glenn Lewis <[email protected]>

gmlewis · 2024-05-30T20:33:23Z

Please re-run step 4 in CONTRIBUTING.md to re-generate the auto-generated files... then the tests should all pass.
You can also test locally before you push by running go test ./...

…oBypassActor, regenerated files

gmlewis

Thank you, @Matthew-Reidy !
LGTM.

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

tomfeigin · 2024-06-11T10:05:07Z

github/repos_rules.go

@@ -395,6 +395,24 @@ type Ruleset struct {
 	Rules        []*RepositoryRule  `json:"rules,omitempty"`
 }

+// rulesetNoOmitBypassActors represents a GitHub ruleset object. The struct does not omit bypassActors if the field is nil or an empty array is passed.
+type rulesetNoOmitBypassActors struct {


I think that you can embed the entire Ruleset object and shadow just the bypass_actors

type rulesetNoOmitBypassActors struct { Ruleset BypassActors []*BypassActor `json:"bypass_actors"` }

example: https://goplay.tools/snippet/7lHZci7LW0y

@gmlewis WDYT?

Then you can use it like so:

func (s *RepositoriesService) UpdateRulesetNoBypassActor(...rs *Ruleset... rsNoBypassActor := &rulesetNoOmitBypassActors{} if rs != nil { rsNoBypassActor := &rulesetNoOmitBypassActors{Ruleset: *rs} }

Sounds great, @tomfeigin - as long as we have a unit test that demonstrates this behavior (meaning a BypassActors=nil still gets passed through in JSON as "bypass_actors":[] ).

github/repos_rules.go

lewismiddleton · 2024-07-08T09:57:54Z

This would be useful for me as I'm tracking a bug in the GitHub terraform provider that I think links to #3137.

@Matthew-Reidy are you planning on picking this up again?

Matthew-Reidy · 2024-07-09T15:16:37Z

I can fix the spelling mistake/nit but as for the other proposed change, whats the justification? The solution in place now on this PR seems to do the same things and has testing evidence behind it. Thanks.

tomfeigin

My comments are only relevant for styling, let's merge it

gmlewis · 2024-07-09T18:26:47Z

Thank you, @tomfeigin and @Matthew-Reidy !
@tomfeigin - after this is merged, feel free to create a PR to improve the styling if you wish.

Merging.

This is a quirk upstream, where they introduced a new function `UpdateRulesetNoBypassActor` to work around the issue of `UpdateRuleset` not deleting BypassActors when there's an empty array. google/go-github#3174 Not sure why they decided to introduce a new function instead of just patching the original. Compatibility issues? Is there ever a use case where we need to keep BypassActors around when the non-empty list becomes an empty list?

Matthew-Reidy and others added 13 commits April 17, 2024 16:46

work done on merge queue addition

2a56435

added support for merge queue, altered tests to include merge queue

ff15302

Merge branch 'google:master' into master

aabde61

alphabetized items in repo_rules.go and AUTHORS, removed blank space …

ae78db8

…in repo_rules.go

Update github/repos_rules.go

db98743

Co-authored-by: Daniel Liao <[email protected]>

Merge branch 'google:master' into master

905ca1c

Merge branch 'google:master' into master

0fd71e1

issue3106 changes

2c8a998

added more detail to function comment

13c74db

small gramatical change

6f4c7c1

Merge branch 'google:master' into master

f43553d

Merge branch 'google:master' into master

cadcc02

added extra endpoint for issue 3137

133421c

gmlewis requested changes May 22, 2024

View reviewed changes

changed func param back to Ruleset

33bfd28

gmlewis requested changes May 30, 2024

View reviewed changes

github/repos_rules.go Outdated Show resolved Hide resolved

github/repos_rules.go Outdated Show resolved Hide resolved

Matthew-Reidy and others added 2 commits May 30, 2024 12:59

Update github/repos_rules.go

0feea60

Co-authored-by: Glenn Lewis <[email protected]>

Update github/repos_rules.go

aef7538

Co-authored-by: Glenn Lewis <[email protected]>

gmlewis requested changes May 30, 2024

View reviewed changes

github/repos_rules.go Outdated Show resolved Hide resolved

github/repos_rules.go Outdated Show resolved Hide resolved

Matthew-Reidy and others added 3 commits May 30, 2024 13:29

Update github/repos_rules.go

e34b7b3

Co-authored-by: Glenn Lewis <[email protected]>

Update github/repos_rules.go

0fd05d7

Co-authored-by: Glenn Lewis <[email protected]>

Merge branch 'google:master' into master

2b20337

changed struct to non-exported, code styling change in UpdateRulesetN…

1f7c1ef

…oBypassActor, regenerated files

gmlewis approved these changes May 30, 2024

View reviewed changes

gmlewis added the NeedsReview PR is awaiting a review before merging. label May 30, 2024

tomfeigin reviewed Jun 11, 2024

View reviewed changes

gmlewis added waiting for reply and removed NeedsReview PR is awaiting a review before merging. labels Jul 8, 2024

fixed spelling nit

fa5bba3

tomfeigin approved these changes Jul 9, 2024

View reviewed changes

gmlewis removed the waiting for reply label Jul 9, 2024

gmlewis merged commit 2204dce into google:master Jul 9, 2024
6 of 7 checks passed

gmlewis mentioned this pull request Jul 10, 2024

Bump version of go-github to v63.0.0 #3205

Merged

lewismiddleton mentioned this pull request Jul 16, 2024

[BUG]: github_repository_ruleset can create bypass_actors but cannot delete integrations/terraform-provider-github#2269

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow an empty array of BypassActors in Ruleset struct in CreateRuleset endpoint #3174

Allow an empty array of BypassActors in Ruleset struct in CreateRuleset endpoint #3174

Matthew-Reidy commented May 22, 2024 •

edited by gmlewis

Loading

codecov bot commented May 22, 2024 •

edited

Loading

gmlewis left a comment

Matthew-Reidy commented May 23, 2024 •

edited

Loading

gmlewis commented May 23, 2024

Matthew-Reidy commented May 30, 2024 •

edited

Loading

gmlewis left a comment

Matthew-Reidy commented May 30, 2024

gmlewis left a comment

gmlewis commented May 30, 2024

gmlewis left a comment

tomfeigin Jun 11, 2024

gmlewis Jun 11, 2024 •

edited

Loading

lewismiddleton commented Jul 8, 2024

Matthew-Reidy commented Jul 9, 2024 •

edited

Loading

tomfeigin left a comment

gmlewis commented Jul 9, 2024

Allow an empty array of BypassActors in Ruleset struct in CreateRuleset endpoint #3174

Allow an empty array of BypassActors in Ruleset struct in CreateRuleset endpoint #3174

Conversation

Matthew-Reidy commented May 22, 2024 • edited by gmlewis Loading

codecov bot commented May 22, 2024 • edited Loading

Codecov Report

gmlewis left a comment

Choose a reason for hiding this comment

Matthew-Reidy commented May 23, 2024 • edited Loading

gmlewis commented May 23, 2024

Matthew-Reidy commented May 30, 2024 • edited Loading

gmlewis left a comment

Choose a reason for hiding this comment

Matthew-Reidy commented May 30, 2024

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis commented May 30, 2024

gmlewis left a comment

Choose a reason for hiding this comment

tomfeigin Jun 11, 2024

Choose a reason for hiding this comment

gmlewis Jun 11, 2024 • edited Loading

Choose a reason for hiding this comment

lewismiddleton commented Jul 8, 2024

Matthew-Reidy commented Jul 9, 2024 • edited Loading

tomfeigin left a comment

Choose a reason for hiding this comment

gmlewis commented Jul 9, 2024

Matthew-Reidy commented May 22, 2024 •

edited by gmlewis

Loading

codecov bot commented May 22, 2024 •

edited

Loading

Matthew-Reidy commented May 23, 2024 •

edited

Loading

Matthew-Reidy commented May 30, 2024 •

edited

Loading

gmlewis Jun 11, 2024 •

edited

Loading

Matthew-Reidy commented Jul 9, 2024 •

edited

Loading