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
|
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.
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.
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.
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.
Works for me, I’ll make the change in a bit 😄
There was a problem hiding this comment.
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.
One of the points was to remove the "else" and un-indent the following section, please:
| } else { | |
| } |
There was a problem hiding this comment.
You’re totally right, I think I had a brain freeze. Don’t know why I kept an else with a return in there.
|
Thanks @gmlewis! sorry about the sloppiness earlier with the requested change 🙏 |
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_mergeparameter 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!!