Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ProtectionChanges to contain the remaining possible return values #2486

Merged
merged 4 commits into from Oct 4, 2022
Merged

Update ProtectionChanges to contain the remaining possible return values #2486

merged 4 commits into from Oct 4, 2022

Conversation

eolso
Copy link
Contributor

@eolso eolso commented Oct 4, 2022

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

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

codecov bot commented Oct 4, 2022

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.

Copy link
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.

github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
github/repos.go Outdated Show resolved Hide resolved
@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
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
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
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
Copy link
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants