Add pr_content_check to features #104
Conversation
| installationID, | ||
| config.PrivateKey) | ||
|
|
||
| if tokenErr != nil { |
There was a problem hiding this comment.
probably better to bubble up an error and handle properly than blow up. I know this may have been refactored out, but that now means it's getting eyes on it. My hope is to remove all log.Fatal* calls and panic so we can use Derek as an SDK in the future.
There was a problem hiding this comment.
I wasn't quite sure how the error should be handled. For now, just writing the error to stdout and returning before the token is used to prevent a panic
There was a problem hiding this comment.
How about returning an error from the function?
There was a problem hiding this comment.
I agree that returning the error and letting the main.go handle all the errors is probably the right way to handle this. However, the way it is implemented here is consistent with the rest of the pullRequestHandler.go file.
I think the change to return the error should be in a follow up PR to make that change across the entire file. It would make for a cleaner commit/change log and keep this change set focused on checking the PR description.
| log.Fatalf("%s limit: %d, remaining: %d", assignLabelErr, res.Limit, res.Remaining) | ||
| } | ||
|
|
||
| body := `Thank you for your contribution. I've just checked and your Pull Request doesn't appear to have any description. |
There was a problem hiding this comment.
Seems consistent with the previous messages we used.
@rgee0 do you have any input on this?
There was a problem hiding this comment.
I don't quite have that English "personality", so I'd appreciate input into proper phrasing 😄
I know Derek's personality is one of his strong points!
There was a problem hiding this comment.
The two comments are very similar. Is there a way we can extract the main part and just update the 'problem detail' when using it?
Thank you for your contribution. I've just checked and <problem detail>. That's something we need before your Pull Request can be merged. Please see our [contributing guide]( + contributingURL + ).
There was a problem hiding this comment.
If it's ok, we could do this in a follow up PR. I think there's more to think about than this one instance.
I may have a solution you can see here: https://play.golang.org/p/FByvDuUR4g2
Not sure where the struct would be defined though. I was thinking in a new file within the handlers package so that it could be used in other handlers.
main.go
Outdated
| deleted = "deleted" | ||
| dcoCheck = "dco_check" | ||
| comments = "comments" | ||
| deleted = "deleted" |
There was a problem hiding this comment.
Maybe the opened const could go here if it's not present already?
There was a problem hiding this comment.
added the const in the pullRequestHandler.go file as that's the only place it's used. Similar to these consts which are only used in the main.go file.
Happy to move it if you'd prefer all the const definitions are in one place.
handler/pullRequestHandler.go
Outdated
| @@ -143,3 +196,7 @@ func hasUnsigned(req types.PullRequestOuter, client *github.Client) (bool, error | |||
| func isSigned(msg string) bool { | |||
| return strings.Contains(msg, "Signed-off-by:") | |||
| } | |||
|
|
|||
| func missingDescription(pr types.PullRequest) bool { | |||
There was a problem hiding this comment.
Can you call this hasDescription please?
There was a problem hiding this comment.
updated. Changing the name redefines this function, so I've updated it to return > 0 rather than the previous implementation of == 0
I've updated the calling function and the tests to reflect this new change
|
This is not strictly a Alex |
|
I would vote for |
This commit adds a new feature for pr_description_required This feature will verify that the incoming PR has something in the description (aka body) If there is no description, the invalid label will be applied and a comment will be added to the PR with a link to the contributing guide. Signed-off-by: Burton Rheutan <[email protected]>
|
@alexellis This is ready now. I've updated the feature name to the even better, more descriptive: I've also got the version of Derek deployed and installed to test end to end on one of my repos. PR Missing a description: PR with description: and finally, a PR with unsigned commits, and no description: |
|
Before this getting merged I would suggest to add |
|
@martindekov can we add that after the merge? |
|
Yeah ^ it was just suggestion |
Description
This commit adds a new feature for pr_content_check
This feature will verify that the incoming PR has
something in the description (aka body)
If there is no description, the invalid label will be applied
and a comment will be added to the PR with a link to the
contributing guide.
Signed-off-by: Burton Rheutan [email protected]
Motivation and Context
Resolves Mark PRs as invalid which have no description #103
How Has This Been Tested?
Unit tests added and passing
Types of changes
Checklist:
git commit -s