From 2b5b016f1ef79e5ad94688470c88b24756e1a513 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 15 Dec 2022 17:39:58 +0100 Subject: [PATCH 1/6] danger: support partial backport label Treat the Backport::Partial label as a backport as well. (cherry picked from commit 1c0c1ba8b9a280c53dd0a40141e2dd69960359b9) --- dangerfile.py | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index a0e0f6b9b0..8b1a821534 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -46,7 +46,8 @@ 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 +is_backport = "Backport" in mr_labels or "Backport::Partial" in mr_labels +is_full_backport = is_backport and "Backport::Partial" not in mr_labels gl = gitlab.Gitlab( url=f"https://{os.environ['CI_SERVER_HOST']}", @@ -140,7 +141,7 @@ 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: + if is_backport 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." @@ -156,28 +157,28 @@ if not danger.gitlab.mr.milestone: fail("Please assign this merge request to a milestone.") ############################################################################### -# VERSION LABELS +# BACKPORT & VERSION LABELS ############################################################################### # # FAIL if any of the following is true for the merge request: # -# * The "Backport" label is set and the number of version labels set is +# * The MR is marked as Backport and the number of version labels set is # different than 1. (For backports, the version label is used for indicating # its target branch. This is a rather ugly attempt to address a UI # deficiency - the target branch for each MR is not visible on milestone # dashboards.) # -# * Neither the "Backport" label nor any version label is set. (If the merge -# request is not a backport, version labels are used for indicating +# * The MR is not marked as "Backport" nor any version label is set. (If the +# merge request is not a backport, version labels are used for indicating # backporting preferences.) version_labels = [l for l in mr_labels if l.startswith("v9.")] -if backport_label_set and len(version_labels) != 1: +if is_backport and len(version_labels) != 1: fail( - "The *Backport* label is set for this merge request. " + "This MR was marked as *Backport*. " "Please also set exactly one version label (*v9.x*)." ) -if not backport_label_set and not version_labels: +if not is_backport and not version_labels: fail( "If this merge request is a backport, set the *Backport* label and " "a single version label (*v9.x*) indicating the target branch. " From cb6ba18aaad7ce34a386fe9b31b525e285e5bc41 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 15 Dec 2022 17:45:54 +0100 Subject: [PATCH 2/6] danger: ensure target branch is in the MR title Having the MR title clearly marked in its title can be very useful when looking through older issues/MRs. This check also ensures that the version from the version label matches the proper version branch (i.e. v9.16 must be marked with [v9_16]). (cherry picked from commit 14b027cf830020cca6a57f0281bb6da73d118483) --- dangerfile.py | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index 8b1a821534..365fe01a1c 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -171,13 +171,23 @@ if not danger.gitlab.mr.milestone: # * The MR is not marked as "Backport" nor any version label is set. (If the # merge request is not a backport, version labels are used for indicating # backporting preferences.) +# +# * The Backport MR doesn't have target branch in the merge request title. version_labels = [l for l in mr_labels if l.startswith("v9.")] -if is_backport and len(version_labels) != 1: - fail( - "This MR was marked as *Backport*. " - "Please also set exactly one version label (*v9.x*)." - ) +if is_backport: + if len(version_labels) != 1: + fail( + "This MR was marked as *Backport*. " + "Please also set exactly one version label (*v9.x*)." + ) + else: + mr_title_version = f"[{version_labels[0].replace('.', '_')}]" + if mr_title_version not in danger.gitlab.mr.title: + fail( + "Backport MRs must have their target branch in the " + f"title. Please put `{mr_title_version}` in the MR title." + ) if not is_backport and not version_labels: fail( "If this merge request is a backport, set the *Backport* label and " From eab1d810727fb5e6442df6336a41a784b4963c16 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 15 Dec 2022 17:48:34 +0100 Subject: [PATCH 3/6] danger: check backport links to the original MR When doing archeology, it is much easier to find stuff if it's properly linked. This check ensures that backport MR are linked to their original MR via a "Backport of !XXXX" message. The regular expression is fairly broad and has been tested to accept the following variants of the message: Backport of MR !XXXX Backport of: !XXXX backport of mr !XXXX Backport of !XXXX Backport of https://gitlab.isc.org/isc-projects/bind9/-/merge_requests/XXXX (cherry picked from commit 12e0b05738cbd456c8c691fd96364b1ac8b6b259) --- dangerfile.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/dangerfile.py b/dangerfile.py index 365fe01a1c..7f0797232c 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -173,7 +173,13 @@ if not danger.gitlab.mr.milestone: # backporting preferences.) # # * The Backport MR doesn't have target branch in the merge request title. +# +# * The Backport MR doesn't link to the original MR is its description. +BACKPORT_OF_RE = re.compile( + r"Backport\s+of.*(merge_requests/|!)([0-9]+)", flags=re.IGNORECASE +) +backport_desc = BACKPORT_OF_RE.search(danger.gitlab.mr.description) version_labels = [l for l in mr_labels if l.startswith("v9.")] if is_backport: if len(version_labels) != 1: @@ -188,6 +194,11 @@ if is_backport: "Backport MRs must have their target branch in the " f"title. Please put `{mr_title_version}` in the MR title." ) + if backport_desc is None: + fail( + "Backport MRs must link to the original MR. Please put " + "`Backport of MR !XXXX` in the MR description." + ) if not is_backport and not version_labels: fail( "If this merge request is a backport, set the *Backport* label and " From 8ebfcb6b5aaf0d6f699b734e80a77599cbbb07bf Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 15 Dec 2022 17:51:24 +0100 Subject: [PATCH 4/6] danger: check that original MR has been merged When checking a backport MR, ensure that the original MR has been merged already. This is vital for followup checks that verify commit IDs from original commits are present in backport commit messages. (cherry picked from commit 89530f1a1cb2e90b5368605fc317eaed35bf9d1f) --- dangerfile.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/dangerfile.py b/dangerfile.py index 7f0797232c..de7a6cef0c 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -175,6 +175,8 @@ if not danger.gitlab.mr.milestone: # * The Backport MR doesn't have target branch in the merge request title. # # * The Backport MR doesn't link to the original MR is its description. +# +# * The original MR linked to from Backport MR hasn't been merged. BACKPORT_OF_RE = re.compile( r"Backport\s+of.*(merge_requests/|!)([0-9]+)", flags=re.IGNORECASE @@ -199,6 +201,14 @@ if is_backport: "Backport MRs must link to the original MR. Please put " "`Backport of MR !XXXX` in the MR description." ) + else: # backport MR is linked to original MR + original_mr_id = backport_desc.groups()[1] + original_mr = proj.mergerequests.get(original_mr_id) + if original_mr.state != "merged": + fail( + f"Original MR !{original_mr_id} has not been merged. " + "Please re-run `danger` check once it's merged." + ) if not is_backport and not version_labels: fail( "If this merge request is a backport, set the *Backport* label and " From 90b4441e3e61abc90c163d21dd37584eb1786ad7 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 15 Dec 2022 17:52:52 +0100 Subject: [PATCH 5/6] danger: check backport commits for original commit IDs A full backport must have all the commit from the original MR and the original commit IDs must be referenced in the backport commit messages. If the criteria above is not met, the MR should be marked as a partial backport. In that case, any discrepencies are only logged as informative messages rather than failures. (cherry picked from commit c617f97784ff898cfea7bbbc6ab6c92eb409009d) --- dangerfile.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/dangerfile.py b/dangerfile.py index de7a6cef0c..a73e6259c8 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -209,6 +209,28 @@ if is_backport: f"Original MR !{original_mr_id} has not been merged. " "Please re-run `danger` check once it's merged." ) + else: # check for commit IDs once original MR is merged + original_mr_commits = list(original_mr.commits(all=True)) + backport_mr_commits = list(mr.commits(all=True)) + for orig_commit in original_mr_commits: + for backport_commit in backport_mr_commits: + if orig_commit.id in backport_commit.message: + break + else: + msg = ( + f"Commit {orig_commit.id} from original MR !{original_mr_id} " + "is not referenced in any of the backport commits." + ) + if not is_full_backport: + message(msg) + else: + msg += ( + " Please use `-x` when cherry-picking to include " + "the full original commit ID. Alternately, use the " + "`Backport::Partial` label if not all original " + "commits are meant to be backported." + ) + fail(msg) if not is_backport and not version_labels: fail( "If this merge request is a backport, set the *Backport* label and " From a11bcfa8ba729df7bd956d7847e5428e02b14677 Mon Sep 17 00:00:00 2001 From: Tom Krizek Date: Thu, 15 Dec 2022 17:55:54 +0100 Subject: [PATCH 6/6] danger: remove obsolete check for cherry pick msg With proper backport commit detection, this check has been made redundant. (cherry picked from commit e8a5ebaee508c216174624fbc37414ea2dcc5b99) --- dangerfile.py | 7 ------- 1 file changed, 7 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index a73e6259c8..76fe5e91d2 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -91,8 +91,6 @@ 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]" @@ -141,11 +139,6 @@ for commit in danger.git.commits: f"Line too long in log message for commit {commit.sha}: " f"```{line}``` ({len(line)} > 72 characters)." ) - if is_backport 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