-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Check for nil pointer in update rule parameters #2854
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2854 +/- ##
=======================================
Coverage 98.06% 98.06%
=======================================
Files 138 138
Lines 12318 12327 +9
=======================================
+ Hits 12080 12089 +9
Misses 162 162
Partials 76 76
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @o-sama!
Please add a new unit test to cover this case.
Yes, we can hold off on the next release until this PR is merged.
github/repos_rules.go
Outdated
if err := json.Unmarshal(*RepositoryRule.Parameters, ¶ms); err != nil { | ||
return err | ||
} | ||
if RepositoryRule.Parameters != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's please change this to make it easier to read:
if RepositoryRule.Parameters == nil {
r.Parameters = nil
return nil
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote it this way since there’s a return nil at the end of the function, the rest of the switch statement also only returns if there’s an error, and just sets r.Parameters
to nil and lets the return nil
at the end of the function take care of it. Do you think I should keep it this way or break the convention for the update case only?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I understand, but I want to highlight the "happy path" and provide the early return which is idiomatic Go.
Please switch to my suggestion, and feel free to put a blank line after the new block if you think it reads better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works for me, I’ll make the change in a bit 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, also added a bit in the update rule constructor to check for empty params since this specific rule is a bit different than others 🙂
github/repos_rules.go
Outdated
if RepositoryRule.Parameters == nil { | ||
r.Parameters = nil | ||
return nil | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the points was to remove the "else" and un-indent the following section, please:
} else { | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You’re totally right, I think I had a brain freeze. Don’t know why I kept an else with a return in there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @o-sama !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
Thanks @gmlewis! sorry about the sloppiness earlier with the requested change 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No problem! Sorry for the delayed responses... Work sometimes gets in the way. 😂 |
Thank you, @liaodaniel ! |
Since discussing this with the GitHub folks, I’ve been told that the
update_allows_fetch_and_merge
parameter for the update rule only applies to forked repos. Note: I was told this is still bugged for forked repos when sending a request through the API, but will be fixed in the near future.Since it only applies to forked repos, the API returns a ruleset which contains:
This doesn't have a parameters field, so when we try to unmarshal the JSON we run into a nil pointer dereference error. No functionality is changed here except to check for a nil pointer before we try to unmarshal the JSON, and if it isn’t included then we just set the parameters to nil.
I didn’t think this needed a change in testing because it doesn’t change any functionality, but I’ll be happy to go and add something if you think otherwise.
@gmlewis I’d really appreciate if this can be part of the upcoming release (which I know is short notice) since I’d love for it to be included in integrations/terraform-provider-github#1808. No worries if it’s too late though, I can just keep it out for now. Thanks!!