Skip to content
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.

Sign up for GitHub

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

Add pr_content_check to features #104

Merged
merged 1 commit into from
Nov 22, 2018
Merged

Add pr_content_check to features #104

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions USER_GUIDE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ redirect: https://raw.githubusercontent.com/openfaas/faas/master/.DEREK.yml

If `dco_check` is specified in the feature list then Derek will inform you when a PR is submitted with commits which have no sign-off. He also adds a label `no-dco`.

### Feature: `pr_description_required`

If `pr_description_required` is specified in the feature list then Derek will inform you that a PR needs a description. He also adds the `invalid` label.

### Feature: `comments`

If `comments` is given in the `features` list then this enables all commenting features:
Expand Down
85 changes: 71 additions & 14 deletions handler/pullRequestHandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,29 +18,25 @@ import (
"github.com/google/go-github/github"
)

const (
prDescriptionRequiredLabel = "invalid"
openedPRAction = "opened"
)

func HandlePullRequest(req types.PullRequestOuter, contributingURL string, config config.Config) {
ctx := context.Background()
token, tokenErr := getAccessToken(config, req.Installation.ID)

token := os.Getenv("personal_access_token")
if len(token) == 0 {

newToken, tokenErr := auth.MakeAccessTokenForInstallation(
config.ApplicationID,
req.Installation.ID,
config.PrivateKey)

if tokenErr != nil {
log.Fatalln(tokenErr.Error())
}

token = newToken
if tokenErr != nil {
fmt.Printf("Error getting installation token: %s\n", tokenErr.Error())
return
}

client := factory.MakeClient(ctx, token, config)

hasUnsignedCommits, err := hasUnsigned(req, client)

if req.Action == "opened" {
if req.Action == openedPRAction {
if req.PullRequest.FirstTimeContributor() == true {
_, res, assignLabelErr := client.Issues.AddLabelsToIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, []string{"new-contributor"})
if assignLabelErr != nil {
Expand Down Expand Up @@ -101,6 +97,63 @@ That's something we need before your Pull Request can be merged. Please see our
}
}

// VerifyPullRequestDescription checks that the PR has anything in the body.
// If there is no body, a label is added and comment posted to the PR with a link to the contributing guide.
func VerifyPullRequestDescription(req types.PullRequestOuter, contributingURL string, config config.Config) {
ctx := context.Background()
token, tokenErr := getAccessToken(config, req.Installation.ID)

if tokenErr != nil {
fmt.Printf("Error getting installation token: %s\n", tokenErr.Error())
return
}

client := factory.MakeClient(ctx, token, config)

if req.Action == openedPRAction {
if !hasDescription(req.PullRequest) {
fmt.Printf("Applying label: %s", prDescriptionRequiredLabel)
_, res, assignLabelErr := client.Issues.AddLabelsToIssue(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, []string{prDescriptionRequiredLabel})
if assignLabelErr != nil {
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.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems consistent with the previous messages we used.

@rgee0 do you have any input on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 + ).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

That's something we need before your Pull Request can be merged. Please see our [contributing guide](` + contributingURL + `).`

comment := &github.IssueComment{
Body: &body,
}

comment, resp, err := client.Issues.CreateComment(ctx, req.Repository.Owner.Login, req.Repository.Name, req.PullRequest.Number, comment)
if err != nil {
log.Fatalf("%s limit: %d, remaining: %d", assignLabelErr, resp.Limit, resp.Remaining)
log.Fatal(err)
}
fmt.Println(comment, resp.Rate)
}
}
}

func getAccessToken(config config.Config, installationID int) (string, error) {
token := os.Getenv("personal_access_token")
if len(token) == 0 {

installationToken, tokenErr := auth.MakeAccessTokenForInstallation(
config.ApplicationID,
installationID,
config.PrivateKey)

if tokenErr != nil {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about returning an error from the function?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return "", tokenErr
}

token = installationToken
}

return token, nil
}

func hasNoDcoLabel(issue *github.Issue) bool {
if issue != nil {
for _, label := range issue.Labels {
Expand Down Expand Up @@ -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 hasDescription(pr types.PullRequest) bool {
return len(strings.TrimSpace(pr.Body)) > 0
}
29 changes: 29 additions & 0 deletions handler/pullRequestHandler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package handler
import (
"testing"

"github.com/alexellis/derek/types"
"github.com/google/go-github/github"
)

Expand Down Expand Up @@ -95,3 +96,31 @@ func Test_hasNoDcoLabel(t *testing.T) {
})
}
}

func Test_hasDescription(t *testing.T) {
var pr = []struct {
title string
body string
expectedBool bool
}{
{
title: "PR with body",
body: "This PR has a body",
expectedBool: true,
},
{
title: "This PR has no body",
body: "",
expectedBool: false,
},
}

for _, test := range pr {
testPr := types.PullRequest{Body: test.body}
hasDescription := hasDescription(testPr)

if hasDescription != test.expectedBool {
t.Errorf("PR missing body - wanted: %t, found: %t", test.expectedBool, hasDescription)
}
}
}
12 changes: 8 additions & 4 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ import (
)

const (
dcoCheck = "dco_check"
comments = "comments"
deleted = "deleted"
dcoCheck = "dco_check"
comments = "comments"
deleted = "deleted"
prDescriptionRequired = "pr_description_required"
)

func main() {
Expand Down Expand Up @@ -80,10 +81,13 @@ func handleEvent(eventType string, bytesIn []byte, config config.Config) error {
return fmt.Errorf("Unable to access maintainers file at: %s/%s", req.Repository.Owner.Login, req.Repository.Name)
}
if req.Action != handler.ClosedConstant {
contributingURL := getContributingURL(derekConfig.ContributingURL, req.Repository.Owner.Login, req.Repository.Name)
if handler.EnabledFeature(dcoCheck, derekConfig) {
contributingURL := getContributingURL(derekConfig.ContributingURL, req.Repository.Owner.Login, req.Repository.Name)
handler.HandlePullRequest(req, contributingURL, config)
}
if handler.EnabledFeature(prDescriptionRequired, derekConfig) {
handler.VerifyPullRequestDescription(req, contributingURL, config)
}
}
break

Expand Down
1 change: 1 addition & 0 deletions types/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ type Owner struct {
type PullRequest struct {
Number int `json:"number"`
AuthorAssociation string `json:"author_association"`
Body string `json:"body"`
}

type InstallationRequest struct {
Expand Down