Fix issue with method defaults #447
Conversation
policy/approval/approve.go
Outdated
| GithubReview: &defaultBool, | ||
| } | ||
| } else { | ||
| if len(methods.Comments) == 0 { |
There was a problem hiding this comment.
I think you could simplify both the if and the else into one. Effectively, if methods == nil, then methods.Comments and methods.GithubReview will also be nil.
There was a problem hiding this comment.
I agree, will make the change.
policy/approval/approve.go
Outdated
| @@ -57,15 +57,19 @@ type RequestReview struct { | |||
|
|
|||
| func (opts *Options) GetMethods() *common.Methods { | |||
| methods := opts.Methods | |||
| defaultBool := true | |||
There was a problem hiding this comment.
Let's use a more descriptive name here and in the other instances where defaultBool has been added. Perhaps defaultGithubReview?
There was a problem hiding this comment.
One comment about the slice handling, but otherwise this looks good.
We'll want to be careful about noting this in the release notes because it is technically a breaking change. As Derek alluded to, if I have a policy like:
options:
methods:
github_review: trueit currently disables comments. In order to keep the same behavior after this PR, I'll have to change the policy to:
options:
methods:
comments: []
github_review: trueThis is consistent with what the docs have always claimed, so I'd count this a bug fix, but people might have been relying on the old broken behavior. We might want to audit at least our internal policies to see if this will impact anything.
policy/common/methods.go
Outdated
| CommentPatterns []Regexp `yaml:"comment_patterns,omitempty"` | ||
| GithubReview bool `yaml:"github_review,omitempty"` | ||
| GithubReviewCommentPatterns []Regexp `yaml:"github_review_comment_patterns,omitempty"` | ||
| Comments *[]string `yaml:"comments,omitempty"` |
There was a problem hiding this comment.
Making this a pointer is not necessary. In Go, slices implicitly work like pointers but we don't usually think about it because a nil slice behaves the same as a non-nil empty slice. When a user does not specify the comments field in configuration, the value will be nil.
There was a problem hiding this comment.
Makes sense. Will revert that back.
policy/approval/approve.go
Outdated
| @@ -82,10 +87,10 @@ func (r *Rule) Trigger() common.Trigger { | |||
|
|
|||
| if r.Requires.Count > 0 { | |||
| m := r.Options.GetMethods() | |||
| if len(m.Comments) > 0 || len(m.CommentPatterns) > 0 { | |||
| if m.Comments != nil && len(*m.Comments) > 0 || len(m.CommentPatterns) > 0 { | |||
There was a problem hiding this comment.
These nil checks and dereferencing are not necessary once m.Comments is reverted back to a []string because len(nil) == 0
policy/common/methods.go
Outdated
| for _, comment := range m.Comments { | ||
| if strings.Contains(commentBody, comment) { | ||
| return true | ||
| if m.Comments != nil { |
There was a problem hiding this comment.
Same idea here with the nil check and dereferencing: you can range over a nil slice without checking first.
When certain attributes are defined under 'methods', the other undefined attributes don't line up with the promised defaults. This PR resolves this issue by checking whether certain attributes were not specified and then adds defaults for them.
Closes #434