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.

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

Disallow at mentions and keywords which close issues in commit messages #9387

Merged
merged 1 commit into from Apr 15, 2019

Conversation

@nikhita
Copy link
Member

commented Sep 13, 2018

Part of #9360

This PR introduces a "invalidcommitmsg" plugin that adds the do-not-merge/invalid-commit-message label on PRs which have commit messages containing @mentions and keywords that automatically close issues.

Adding an explicit hold because this adds a new label and this should go through some other channels first.
/hold

This needs follow-ups:

  • Communicate about the label in appropriate channels: k-dev email
  • Update the labels in label_sync: #12215
  • Add the plugin to the hook binary: #12199
  • Bump prow.
  • Enable the plugin in plugins.yaml: #12236
  • Add info about this in the contributor guide: kubernetes/community#3642
@nikhita

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

Would especially love some feedback on:

  • The plugin and label name. :)

  • The only way to list commits in a PR seems to be through https://developer.github.com/v3/pulls/#list-commits-on-a-pull-request. There was no method related to this in the prow/github client. I added a new method ListPullRequestCommits for it. Ptal.

  • While doing so, I noticed that the API request differentiates between a "github commit" and a "git commit". The API request returns a "github commit" which wraps a "git commit". I also added a new type called RepositoryCommit to follow the "github commit". This is inspired by go-github.

  • This led to modifying the type Commit which is supposed to be the "git commit". The fields in the existing API type do not mirror the fields returned by the GitHub API. I also noticed that Commit is not really used anywhere - it is used in a field in PushEvent but that field is never really utilized anywhere. Is it safe to change this?

  • Most of these fields should be omitempty. To be truly omitempty, they should also be pointers. However, I noticed that most of the existing fields are not pointers but have the omitempty json tag. What should the new API type contain?

@nikhita

This comment has been minimized.

Copy link
Member Author

commented Sep 13, 2018

@nikhita nikhita force-pushed the nikhita:commit-msg-forbid-keywords branch from 3e1fa11 to 430f0b5 Sep 13, 2018

Show resolved Hide resolved prow/github/client.go Outdated
Show resolved Hide resolved prow/plugins/invalidcommitmsg/invalidcommitmsg.go Outdated
Show resolved Hide resolved prow/plugins/invalidcommitmsg/invalidcommitmsg.go Outdated
Show resolved Hide resolved prow/github/types.go Outdated
@liggitt

This comment has been minimized.

Copy link
Member

commented Sep 13, 2018

do we have any cache capability? once we determine validity of a given sha, we never need to compute it again

@nikhita

This comment has been minimized.

Copy link
Member Author

commented Oct 1, 2018

quick update here: haven't forgotten this, will try to get to it this week. :)

@spiffxp

This comment has been minimized.

Copy link
Member

commented Dec 7, 2018

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from spiffxp Dec 7, 2018

@BenTheElder

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

searching for life signs 🛸 / clearing my PR dashboard

@cblecker

This comment has been minimized.

Copy link
Member

commented Jan 5, 2019

/uncc
Please ask for review when ready :)

@k8s-ci-robot k8s-ci-robot removed the request for review from cblecker Jan 5, 2019

@fejta

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2019

Any chance we can get this one? Will be super useful!

@nikhita nikhita force-pushed the nikhita:commit-msg-forbid-keywords branch from 430f0b5 to 9e0acc7 Apr 2, 2019

@k8s-ci-robot k8s-ci-robot added sig/testing and removed size/XL labels Apr 2, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

LGTM label has been added.

Git tree hash: f8c4088098530c694c7bae950f1c9a6e2cef5d42

@nikhita nikhita force-pushed the nikhita:commit-msg-forbid-keywords branch from 355acaf to 47f8b76 Apr 2, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 2, 2019

@cjwagner
Copy link
Member

left a comment

Nothing, but nits.
/hold

Show resolved Hide resolved prow/plugins/invalidcommitmsg/invalidcommitmsg.go Outdated
Show resolved Hide resolved prow/plugins/invalidcommitmsg/invalidcommitmsg.go Outdated
}

shouldComment := pr.Action == github.PullRequestActionSynchronize ||
pr.Action == github.PullRequestActionOpened

This comment has been minimized.

Copy link
@cjwagner

cjwagner Apr 2, 2019

Member

Please add a (code) comment to indicate why we only want to comment (on GH) for these specific actions.

This comment has been minimized.

Copy link
@nikhita

nikhita Apr 3, 2019

Author Member

Thinking more about this, I removed the condition because "reopen" actions sometimes make it easy to retrigger bot automation in the case of a dropped webhook, etc.


var (
closeIssueRegex = regexp.MustCompile(`((?i)(clos(?:e[sd]?))|(fix(?:(es|ed)?))|(resolv(?:e[sd]?)))[\s:]+(\w+/\w+)?#(\d+)`)
atMentionRegex = regexp.MustCompile(`\s([@][\w_-]+)`)

This comment has been minimized.

Copy link
@cjwagner

cjwagner Apr 2, 2019

Member

\b is probably more appropriate than \s here.

This comment has been minimized.

Copy link
@nikhita

nikhita Apr 3, 2019

Author Member

Did you mean \B? Updated it to use \B, PTAL.

This comment has been minimized.

Copy link
@cjwagner

cjwagner Apr 3, 2019

Member

I meant \b, but it doesn't work the way I expected in this case because @ does not match \w. https://github.com/google/re2/wiki/Syntax

\B might be the best option then? I'm not sure if there is a practical difference between using \B or (^|\s) here.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

LGTM label has been added.

Git tree hash: 4a63adf9f15e84bed65341f12cd04178c5a239b8

1 similar comment
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 2, 2019

LGTM label has been added.

Git tree hash: 4a63adf9f15e84bed65341f12cd04178c5a239b8

prow: add invalidcommitmsg plugin
The invalidcommitmsg plugin disallows commit messages which contain at mentions
(since that leads to lots of email spam) and keywords which automatically close
issues (like fixes, closes, etc).

@nikhita nikhita force-pushed the nikhita:commit-msg-forbid-keywords branch from 47f8b76 to 93998ee Apr 3, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Apr 3, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2019

LGTM label has been added.

Git tree hash: 67059aaa0f3ae3c02579ae114dd33c20f6cfd023

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 3, 2019

@nikhita

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@cblecker ok to cancel the hold here? :)

@neolit123
Copy link
Member

left a comment

nice!

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, neolit123, nikhita

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@nikhita

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

@cblecker ping :)

@cblecker

This comment has been minimized.

Copy link
Member

commented Apr 12, 2019

LGTM 👍 Thanks for this @nikhita !

@nikhita

This comment has been minimized.

Copy link
Member Author

commented Apr 15, 2019

Cancelling the hold.

/hold cancel

@k8s-ci-robot k8s-ci-robot merged commit 8ac75e2 into kubernetes:master Apr 15, 2019

15 checks passed

cla/linuxfoundation nikhita authorized
Details
pull-test-infra-bazel Job succeeded.
Details
pull-test-infra-gubernator Skipped.
pull-test-infra-lint Job succeeded.
Details
pull-test-infra-verify-bazel Job succeeded.
Details
pull-test-infra-verify-codegen Job succeeded.
Details
pull-test-infra-verify-config Job succeeded.
Details
pull-test-infra-verify-deps Skipped.
pull-test-infra-verify-file-perms Job succeeded.
Details
pull-test-infra-verify-github-spelling Job succeeded.
Details
pull-test-infra-verify-gofmt Job succeeded.
Details
pull-test-infra-verify-govet Job succeeded.
Details
pull-test-infra-verify-labels Skipped.
pull-test-infra-verify-tslint Skipped.
tide In merge pool.
Details

@nikhita nikhita deleted the nikhita:commit-msg-forbid-keywords branch Apr 15, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.