pullrequest-init: Add getters to make PullRequest nil-safe. #1017
Conversation
Adds getters to all funcs so that libraries can safely handle the struct without needing to worry about doing nil checks to avoid panics. This is heavily influenced by the behavior of Go protobufs and other libraries like https://github.com/google/go-github. Also adds test coverage for uploadComments and uploadLabels for nil lists.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wlynch If they are not already assigned, you can assign the PR to them by writing The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
googlebot
added
the
cla: yes
tekton-robot
added
the
size/L
|
/assign @imjasonh |
There was a problem hiding this comment.
Thank you for this, it looks good. The only question I have is weather returning a default value when invoking the getting on a nil may not have unwanted side effects.
It's good to have a nil-safe getters but it might be good in specific cases to check for nil values before using the the getter, to avoid the side effects.
| @@ -265,14 +270,14 @@ func (h *GitHubHandler) updateExistingComments(ctx context.Context, comments map | |||
| merr = multierror.Append(merr, err) | |||
| continue | |||
| } | |||
| } else if dc.Text != ec.GetBody() { | |||
| } else if dc.GetText() != ec.GetBody() { | |||
There was a problem hiding this comment.
If L264 dc, ok := comments[ec.GetID()] returns nil, true the comment ID is in comments but the value is nil, so we're probably in an error situation, since it shouldn't be possible in normal conditions to for the desired comment to be nil. With the nil-safe getter this condition here will be met and we will update the comment to "". I think this is not the desired behaviour.
| if _, _, err := h.Issues.CreateComment(ctx, h.owner, h.repo, h.prNum, c); err != nil { | ||
| h.Logger.Warnf("Error creating comment: %v", err) | ||
| merr = multierror.Append(merr, err) | ||
| } | ||
| } | ||
| return merr | ||
| } | ||
|
|
||
| func githubCommentToTekton(c *github.IssueComment, pathPrefix string) *Comment { |
There was a problem hiding this comment.
Just curious, why did you place this when it's only used to build Comment for testing?
There was a problem hiding this comment.
right, if it's only used for the test, let's add it in test files.
There was a problem hiding this comment.
hmm I have some mixed feelings about this! I have definitely fallen victim to not checking for nil before, so this seems pretty useful, but I think if we're going to do it, we should commit to doing it for all our types. And like @afrittoli said, it may not always make sense to return a default value, tho we could be careful about choosing it. It would probably clean up a lot of code to be able to iterate over lists without worrying they are nil, e.g.
pipeline/pkg/reconciler/v1alpha1/taskrun/validate_resources.go
Lines 27 to 29 in c4858b6
What do other OWNERS think @vdemeester @dlorenc @imjasonh (and soon) @abayer ?
| @@ -41,7 +41,10 @@ func TestFakeGitHubComments(t *testing.T) { | |||
| t.Fatalf("List Issues: wanted [], got %+v, %+v, %v", got, resp, err) | |||
| } | |||
|
|
|||
| if _, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, comment); err != nil { | |||
| comment, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ | |||
There was a problem hiding this comment.
i might just not be seeing it, but I can't find where the comment var was declared before this change :O Was this a bug in the tests? If so that would mean they aren't running in CI and there's no linting happening... 😨
| @@ -41,7 +41,10 @@ func TestFakeGitHubComments(t *testing.T) { | |||
| t.Fatalf("List Issues: wanted [], got %+v, %+v, %v", got, resp, err) | |||
| } | |||
|
|
|||
| if _, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, comment); err != nil { | |||
| comment, _, err := client.Issues.CreateComment(ctx, owner, repo, prNum, &github.IssueComment{ | |||
| if _, _, err := h.Issues.CreateComment(ctx, h.owner, h.repo, h.prNum, c); err != nil { | ||
| h.Logger.Warnf("Error creating comment: %v", err) | ||
| merr = multierror.Append(merr, err) | ||
| } | ||
| } | ||
| return merr | ||
| } | ||
|
|
||
| func githubCommentToTekton(c *github.IssueComment, pathPrefix string) *Comment { |
There was a problem hiding this comment.
right, if it's only used for the test, let's add it in test files.
|
We ended up running into an issue in our 0.5.0 release (#1075 ) that was due to trying to access an attribute on a nil value - I agree with what @vdemeester said about using getters not being very golang-y (is there a proper word for that?) but I also wonder if we can make our types a bit more safe. Seems like this isn't the direction we want to go at the moment tho 🤔 |
|
I'm going to close this for now assuming we don't want to go in this direction but please re-open if I'm wrong! |
Changes
Adds getters to all funcs so that libraries can safely handle the struct
without needing to worry about doing nil checks to avoid panics. This is
heavily influenced by the behavior of Go protobufs and other libraries
like https://github.com/google/go-github.
Also adds test coverage for uploadComments and uploadLabels for nil
lists.
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide
for more details.
Release Notes
None.