From e901342dd9b2636c00948efa3ec09d1f5e9e71cc Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Fri, 4 Nov 2022 13:05:29 +0100 Subject: [PATCH 1/3] Use approve button workflow in danger CI Since the LGTM label was deprecated in favor of using the Approve button in gitlab, adjust the detection in danger bot. Unfortunately, danger-python seems no longer maintained since 2020 and MR approvals aren't available in its Python API (even though they're supported in its Ruby/JS APIs). Going forward, let's use the more comprehensive python-gitlab API. It still makes sense to utilize the danger-python, since it handles the integration with gitlab which doesn't need to be reimplemented as long as it works - same with the other checks. --- dangerfile.py | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index 2f139f79aa..c8b495785e 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -11,8 +11,11 @@ # information regarding copyright ownership. ############################################################################ +import os import re +import gitlab + # Helper functions and variables @@ -44,6 +47,13 @@ modified_files = danger.git.modified_files mr_labels = danger.gitlab.mr.labels target_branch = danger.gitlab.mr.target_branch +gl = gitlab.Gitlab( + url=f"https://{os.environ['CI_SERVER_HOST']}", + private_token=os.environ["DANGER_GITLAB_API_TOKEN"], +) +proj = gl.projects.get(os.environ["CI_PROJECT_ID"]) +mr = proj.mergerequests.get(os.environ["CI_MERGE_REQUEST_IID"]) + ############################################################################### # COMMIT MESSAGES ############################################################################### @@ -167,15 +177,16 @@ if not backport_label_set and not version_labels: # remind developers about the need to set the latter on merge requests which # passed review.) +approved = mr.approvals.get().approved if "Review" not in mr_labels: warn( "This merge request does not have the *Review* label set. " "Please set it if you would like the merge request to be reviewed." ) -elif "LGTM (Merge OK)" not in mr_labels: +elif not approved: warn( "This merge request is currently in review. " - "It should not be merged until it is marked with the *LGTM* label." + "It should not be merged until it is approved." ) ############################################################################### From 402b11431c1ea417ffcd586c547f303c127b655e Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Mon, 7 Nov 2022 14:18:55 +0100 Subject: [PATCH 2/3] Detect work-in-progress commits in danger CI To avoid accidentally merging unfinished work, detect prohibited keywords at the start of the subject line. If the first word is any of the following, fail the check: WIP, wip, DROP, drop, TODO, todo The only slightly controversial is the lowercase "drop" which might have a legitimate use - seems like four commits in the history used it as a start of a sentence. However, since people commonly use "drop" to indicate a commit should be dropped before merging, let's prohibit it as well. In case of false-positive, "Drop" with a capitalized first letter can always be used. --- dangerfile.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dangerfile.py b/dangerfile.py index c8b495785e..a2b90bf0ce 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -62,6 +62,9 @@ mr = proj.mergerequests.get(os.environ["CI_MERGE_REQUEST_IID"]) # # * The subject line starts with "fixup!" or "Apply suggestion". # +# * The subject line starts with a prohibited word indicating a work in +# progress commit (e.g. "WIP"). +# # * The subject line contains a trailing dot. # # * There is no empty line between the subject line and the log message. @@ -87,6 +90,9 @@ mr = proj.mergerequests.get(os.environ["CI_MERGE_REQUEST_IID"]) # "[2]", etc.) which allows e.g. long URLs to be included in the # commit log message. +PROHIBITED_WORDS_RE = re.compile( + "^(WIP|wip|DROP|drop|DROPME|checkpoint|experiment|TODO|todo)[^a-zA-Z]" +) fixup_error_logged = False for commit in danger.git.commits: message_lines = commit.message.splitlines() @@ -99,6 +105,12 @@ for commit in danger.git.commits: "Please squash them before merging." ) fixup_error_logged = True + match = PROHIBITED_WORDS_RE.search(subject) + if match: + fail( + f"Prohibited keyword `{match.groups()[0]}` detected " + f"at the start of a subject line in commit {commit.sha}." + ) if len(subject) > 72 and not subject.startswith("Merge branch "): warn( f"Subject line for commit {commit.sha} is too long: " From 5ecb277090958cda2ec346ddde48849a3aaade33 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Tue, 8 Nov 2022 10:53:09 +0100 Subject: [PATCH 3/3] Check for cherry pick message in backport commits in danger CI Using the -x option for cherry pick makes it easy to link commits across branches and it is recommended to use for all backport commits (with exceptions -- thus a warning level rather than failure). --- dangerfile.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dangerfile.py b/dangerfile.py index a2b90bf0ce..a0e0f6b9b0 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -46,6 +46,7 @@ release_notes_regex = re.compile(r"doc/(arm|notes)/notes-.*\.(rst|xml)") modified_files = danger.git.modified_files mr_labels = danger.gitlab.mr.labels target_branch = danger.gitlab.mr.target_branch +backport_label_set = "Backport" in mr_labels gl = gitlab.Gitlab( url=f"https://{os.environ['CI_SERVER_HOST']}", @@ -89,6 +90,8 @@ mr = proj.mergerequests.get(os.environ["CI_MERGE_REQUEST_IID"]) # - lines which contain references (i.e. those starting with "[1]", # "[2]", etc.) which allows e.g. long URLs to be included in the # commit log message. +# +# * There is no "cherry picked from X" message in Backport commits. PROHIBITED_WORDS_RE = re.compile( "^(WIP|wip|DROP|drop|DROPME|checkpoint|experiment|TODO|todo)[^a-zA-Z]" @@ -137,6 +140,11 @@ for commit in danger.git.commits: f"Line too long in log message for commit {commit.sha}: " f"```{line}``` ({len(line)} > 72 characters)." ) + if backport_label_set and "cherry picked from commit" not in commit.message: + warn( + f"`cherry picked from commit...` message missing in commit {commit.sha}. " + "Please use `-x` option with `git cherry-pick` or remove the `Backport` label." + ) ############################################################################### # MILESTONE @@ -163,7 +171,6 @@ if not danger.gitlab.mr.milestone: # request is not a backport, version labels are used for indicating # backporting preferences.) -backport_label_set = "Backport" in mr_labels version_labels = [l for l in mr_labels if l.startswith("v9.")] if backport_label_set and len(version_labels) != 1: fail(