Skip to content

Update ProtectionChanges to contain the remaining possible return values#2486

Merged
gmlewis merged 4 commits intogoogle:masterfrom
eolso:update-branch-protection-changes
Oct 4, 2022
Merged

Update ProtectionChanges to contain the remaining possible return values#2486
gmlewis merged 4 commits intogoogle:masterfrom
eolso:update-branch-protection-changes

Conversation

@eolso
Copy link
Copy Markdown
Contributor

@eolso eolso commented Oct 4, 2022 •
edited
Loading

Fixes issue #2485. The From values only seem to come in the variants of string, bool or []string, so it's possible these objects could be consolidated into something more generic maybe.
e.g.

type FromString struct {
    From *string `json:"from,omitempty"`
}

type FromBool struct {
    From *bool `json:"from,omitempty"`
}

type FromStringSlice struct {
    From []string `json:"from,omitempty"`
}

This went against the current pattern so I made everything a <Value>Changes struct instead :D

@google-cla
Copy link
Copy Markdown

google-cla bot commented Oct 4, 2022

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 4, 2022 •
edited
Loading

Codecov Report

Merging #2486 (e7143e7) into master (08819b7) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2486   +/-   ##
=======================================
  Coverage   98.03%   98.03%           
=======================================
  Files         122      122           
  Lines       10705    10705           
=======================================
  Hits        10495    10495           
  Misses        144      144           
  Partials       66       66
Impacted Files Coverage Δ
github/repos.go 98.67% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

gmlewis
gmlewis requested changes Oct 4, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @eolso !
Just a few minor tweaks, please.

@gmlewis gmlewis added the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Oct 4, 2022
@eolso
Copy link
Copy Markdown
Contributor Author

eolso commented Oct 4, 2022

Fixed!

@gmlewis the structs for the fields AuthorizedActorNames and AuthorizedActorsOnly could be left alone to prevent any breaking changes if that's preferred? I only renamed them to keep the pattern consistent in the struct. I also only added the Changes to the end of each type name because RequiredStatusChecks was already defined elsewhere 😅

@gmlewis
Copy link
Copy Markdown
Collaborator

gmlewis commented Oct 4, 2022

Fixed!

@gmlewis the structs for the fields AuthorizedActorNames and AuthorizedActorsOnly could be left alone to prevent any breaking changes if that's preferred? I only renamed them to keep the pattern consistent in the struct. I also only added the Changes to the end of each type name because RequiredStatusChecks was already defined elsewhere sweat_smile

Thank you, @eolso - I'll review this after work.

Meanwhile, yes, I prefer to avoid breaking API changes if possible... and in this case I think it is probably fine to have the slight inconsistencies... so maybe reverting these two name changes would be preferable. If any other contributors have other opinions, I'm happy to discuss it.

@eolso
Copy link
Copy Markdown
Contributor Author

eolso commented Oct 4, 2022

Agreed, "consistency" can wait. Reverted breaking changes 😄

@gmlewis gmlewis removed the Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). label Oct 4, 2022
gmlewis
gmlewis approved these changes Oct 4, 2022
View reviewed changes
Copy link
Copy Markdown
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

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

Thank you, @eolso !
LGTM.
Merging.

@gmlewis gmlewis merged commit 330d92d into google:master Oct 4, 2022
@gmlewis gmlewis mentioned this pull request Oct 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gmlewis gmlewis gmlewis approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@eolso @gmlewis