2
0
mirror of https://gitlab.isc.org/isc-projects/bind9 synced 2025-08-22 10:10:06 +00:00
bind/dangerfile.py
Michał Kępień 80ec57f198
Warn about auto-generated merge request titles
Merge request titles auto-generated by GitLab are often a source of
confusion regarding the actual contents of a given merge request.  Warn
for merge requests containing titles that look like auto-generated ones.
2024-06-03 13:07:21 +02:00

575 lines
23 KiB
Python

############################################################################
# Copyright (C) Internet Systems Consortium, Inc. ("ISC")
#
# SPDX-License-Identifier: MPL-2.0
#
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, you can obtain one at https://mozilla.org/MPL/2.0/.
#
# See the COPYRIGHT file distributed with this work for additional
# information regarding copyright ownership.
############################################################################
import glob
import os
import re
import gitlab
# Helper functions and variables
def added_lines(target_branch, paths):
import subprocess
# Hazard fetches the target branch itself, so there is no need to fetch it
# explicitly using `git fetch --depth 1000 origin <target_branch>`. The
# refs/remotes/origin/<target_branch> ref is also expected to be readily
# usable by the time this file is executed.
diff = subprocess.check_output(
["/usr/bin/git", "diff", f"origin/{target_branch}...", "--"] + paths
)
added_lines = []
for line in diff.splitlines():
if line.startswith(b"+") and not line.startswith(b"+++"):
added_lines.append(line)
return added_lines
def lines_containing(lines, string):
return [l for l in lines if bytes(string, "utf-8") in l]
changes_issue_or_mr_id_regex = re.compile(rb"\[(GL [#!]|RT #)[0-9]+\]")
relnotes_issue_or_mr_id_regex = re.compile(rb":gl:`[#!][0-9]+`")
release_notes_regex = re.compile(r"doc/(arm|notes)/notes-.*\.(rst|xml)")
rdata_regex = re.compile(r"lib/dns/rdata/")
modified_files = danger.git.modified_files
affected_files = (
danger.git.modified_files + danger.git.created_files + danger.git.deleted_files
)
mr_title = danger.gitlab.mr.title
mr_labels = danger.gitlab.mr.labels
source_branch = danger.gitlab.mr.source_branch
target_branch = danger.gitlab.mr.target_branch
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']}",
private_token=os.environ["BIND_TEAM_API_TOKEN"],
)
proj = gl.projects.get(os.environ["CI_PROJECT_ID"])
mr = proj.mergerequests.get(os.environ["CI_MERGE_REQUEST_IID"])
###############################################################################
# MERGE REQUEST INFORMATION
###############################################################################
#
# - WARN if the merge request's title looks like an auto-generated one.
if mr_title.replace("Draft: ", "").startswith('Resolve "'):
warn(
f"This merge request's title (`{mr_title}`) looks like an "
"auto-generated one. Please change it so that it accurately "
"describes the changes contained in this merge request."
)
###############################################################################
# BRANCH NAME
###############################################################################
#
# - FAIL if the source branch of the merge request includes an old-style
# "-v9_x" or "-v9.x" suffix.
branch_name_regex = r"^(?P<base>.*?)(?P<suffix>-v9[_.](?P<version>[0-9]+))$"
match = re.match(branch_name_regex, source_branch)
if match:
fail(
f"Source branch name `{source_branch}` includes an old-style version "
f"suffix (`{match.group('suffix')}`). Using such suffixes is now "
"deprecated. Please resubmit the merge request with the branch name "
f"set to `{match.group('base')}-bind-9.{match.group('version')}`."
)
###############################################################################
# COMMIT MESSAGES
###############################################################################
#
# - FAIL if any of the following is true for any commit on the MR branch:
#
# * The subject line starts with "fixup!", "amend!" 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.
#
# - WARN if any of the following is true for any commit on the MR branch:
#
# * The length of the subject line for a non-merge commit exceeds 72
# characters.
#
# * There is no log message present (i.e. commit only has a subject) and
# the subject line does not contain any of the following strings:
# "fixup!", " CHANGES ", " release note".
#
# * Any line of the log message is longer than 72 characters. This rule is
# not evaluated for:
#
# - lines starting with four spaces, which allows long lines to be
# included in the commit log message by prefixing them with four
# spaces (useful for pasting compiler warnings, static analyzer
# messages, log lines, etc.),
#
# - 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.
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()
subject = message_lines[0]
is_merge = subject.startswith("Merge branch ")
is_fixup = (
subject.startswith("fixup!")
or subject.startswith("amend!")
or subject.startswith("Apply suggestion")
)
if not fixup_error_logged and is_fixup:
fail(
"Fixup commits are still present in this merge request. "
"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 is_merge and not is_fixup:
warn(
f"Subject line for commit {commit.sha} is too long: "
f"```{subject}``` ({len(subject)} > 72 characters)."
)
if subject[-1] == ".":
fail(f"Trailing dot found in the subject of commit {commit.sha}.")
if len(message_lines) > 1 and message_lines[1]:
fail(f"No empty line after subject for commit {commit.sha}.")
if (
len(message_lines) < 3
and "fixup! " not in subject
and "CHANGES " not in subject
and "release note" not in subject.lower()
and "GL #" not in subject
):
warn(f"Please write a log message for commit {commit.sha}.")
for line in message_lines[2:]:
if (
len(line) > 72
and not line.startswith(" ")
and not re.match(r"\[[0-9]+\]", line)
):
warn(
f"Line too long in log message for commit {commit.sha}: "
f"```{line}``` ({len(line)} > 72 characters)."
)
###############################################################################
# MILESTONE
###############################################################################
#
# FAIL if the merge request is not assigned to any milestone.
if not danger.gitlab.mr.milestone:
fail("Please assign this merge request to a milestone.")
###############################################################################
# BACKPORT & VERSION LABELS
###############################################################################
#
# FAIL if any of the following is true for the merge request:
#
# * The MR is marked as a Backport and has any "Affects v9.x" label(s) set.
#
# * 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.)
#
# * 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.
#
# * 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
)
VERSION_LABEL_RE = re.compile(r"v9.([0-9]+)(-S)?")
version_labels = [l for l in mr_labels if l.startswith("v9.")]
affects_labels = [l for l in mr_labels if l.startswith("Affects v9.")]
if is_backport:
if affects_labels:
fail("Backports must not have any *Affects v9.x* labels set.")
if len(version_labels) != 1:
fail(
"This MR was marked as *Backport*. "
"Please also set exactly one version label (*v9.x*)."
)
else:
minor_ver, edition = VERSION_LABEL_RE.search(version_labels[0]).groups()
edition = "" if edition is None else edition
title_re = f"^\\[9.{minor_ver}{edition}\\]"
match = re.search(title_re, mr_title)
if match is None:
fail(
"Backport MRs must have their target version in the title. "
f"Please put `[9.{minor_ver}{edition}]` at the start of the MR title."
)
backport_desc = BACKPORT_OF_RE.search(danger.gitlab.mr.description or "")
if backport_desc is None:
fail(
"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."
)
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)
else:
if 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. "
"If not, set version labels for all targeted backport branches."
)
if not affects_labels:
warn(
"Set `Affects v9.` label(s) for all versions that are affected by "
"the issue which this MR addresses."
)
###############################################################################
# OTHER LABELS
###############################################################################
#
# WARN if any of the following is true for the merge request:
#
# * The "Review" label is not set. (It may be intentional, but rarely is.)
#
# * The "Review" label is set, but the "LGTM" label is not set. (This aims to
# 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 not approved:
warn(
"This merge request is currently in review. "
"It should not be merged until it is approved."
)
###############################################################################
# 'CHANGES' FILE
###############################################################################
#
# FAIL if any of the following is true:
#
# * The merge request does not update the CHANGES file, but it does not have
# the "No CHANGES" label set. (This attempts to ensure that the author of
# the MR did not forget about adding a CHANGES entry.)
#
# * The merge request updates the CHANGES file, but it has the "No CHANGES"
# label set. (This attempts to ensure that the "No CHANGES" label is used in
# a sane way.)
#
# * The merge request adds any placeholder entries to the CHANGES file, but it
# does not target the "main" branch.
#
# * The merge request adds a new CHANGES entry that is not a placeholder and
# does not contain any GitLab/RT issue/MR identifiers.
changes_modified = "CHANGES" in modified_files or "CHANGES.SE" in modified_files
no_changes_label_set = "No CHANGES" in mr_labels
if not changes_modified and not no_changes_label_set:
fail(
"This merge request does not modify `CHANGES`. "
"Add a `CHANGES` entry or set the *No CHANGES* label."
)
if changes_modified and no_changes_label_set:
fail(
"This merge request modifies `CHANGES`. "
"Revert `CHANGES` modifications or unset the *No Changes* label."
)
changes_added_lines = added_lines(target_branch, ["CHANGES", "CHANGES.SE"])
placeholders_added = lines_containing(changes_added_lines, "[placeholder]")
identifiers_found = filter(changes_issue_or_mr_id_regex.search, changes_added_lines)
if changes_added_lines:
if placeholders_added:
if target_branch != "main":
fail(
"This MR adds at least one placeholder entry to `CHANGES`. "
"It should be targeting the `main` branch."
)
elif not any(identifiers_found):
fail("No valid issue/MR identifiers found in added `CHANGES` entries.")
###############################################################################
# RELEASE NOTES
###############################################################################
#
# - FAIL if any of the following is true:
#
# * The merge request does not update release notes and has the "Release
# Notes" label set. (This attempts to point out missing release notes.)
#
# * The merge request updates release notes but does not have the "Release
# Notes" label set. (This ensures that merge requests updating release
# notes can be easily found using the "Release Notes" label.)
#
# * A file was added to or deleted from the lib/dns/rdata/ subdirectory but
# release notes were not modified. This is probably a mistake because new
# RR types are a user-visible change (and so is removing support for
# existing ones).
#
# * "Release notes" and "No CHANGES" labels are both set at the same time.
# (If something is worth a release note, it should surely show up in
# CHANGES.) MRs with certain labels set ("Documentation", "Release") are
# exempt because these are typically used during release process.
#
# - WARN if any of the following is true:
#
# * This merge request does not update release notes and has the "Customer"
# label set. (Except for trivial changes, all merge requests which may
# be of interest to customers should include a release note.)
#
# * This merge request updates release notes, but no GitLab/RT issue/MR
# identifiers are found in the lines added to the release notes by this
# MR.
release_notes_regex = re.compile(r"doc/(arm|notes)/notes-.*\.(rst|xml)")
release_notes_changed = list(filter(release_notes_regex.match, affected_files))
release_notes_label_set = "Release Notes" in mr_labels
if not release_notes_changed:
if release_notes_label_set:
fail(
"This merge request has the *Release Notes* label set. "
"Update release notes or unset the *Release Notes* label."
)
elif "Customer" in mr_labels:
warn(
"This merge request has the *Customer* label set. "
"Update release notes unless the changes introduced are trivial."
)
rdata_types_add_rm = list(
filter(rdata_regex.match, danger.git.created_files + danger.git.deleted_files)
)
if rdata_types_add_rm:
fail(
"This merge request adds new files to `lib/dns/rdata/` and/or "
"deletes existing files from that directory, which almost certainly "
"means that it adds support for a new RR type or removes support "
"for an existing one. Please add a relevant release note."
)
if release_notes_changed and not release_notes_label_set:
fail(
"This merge request modifies release notes. "
"Revert release note modifications or set the *Release Notes* label."
)
if (
release_notes_label_set
and no_changes_label_set
and not ("Documentation" in mr_labels or "Release" in mr_labels)
):
fail(
"This merge request is labeled with both *Release notes* and *No CHANGES*. "
"A user-visible change should also be mentioned in the `CHANGES` file."
)
if release_notes_changed:
modified_or_new_files = danger.git.modified_files + danger.git.created_files
release_notes_added = list(filter(release_notes_regex.match, modified_or_new_files))
notes_added_lines = added_lines(target_branch, release_notes_added)
identifiers_found = filter(relnotes_issue_or_mr_id_regex.search, notes_added_lines)
if notes_added_lines and not any(identifiers_found):
warn("No valid issue/MR identifiers found in added release notes.")
else:
notes_added_lines = []
###############################################################################
# CVE IDENTIFIERS
###############################################################################
#
# FAIL if the merge request adds a CHANGES entry of type [security] and a CVE
# identifier is missing from either the added CHANGES entry or the added
# release note.
if lines_containing(changes_added_lines, "[security]"):
if not lines_containing(changes_added_lines, "(CVE-20"):
fail(
"This merge request fixes a security issue. "
"Please add a CHANGES entry which includes a CVE identifier."
)
if not lines_containing(notes_added_lines, ":cve:`20"):
fail(
"This merge request fixes a security issue. "
"Please add a release note which includes a CVE identifier."
)
###############################################################################
# PAIRWISE TESTING
###############################################################################
#
# FAIL if the merge request adds any new ./configure switch without an
# associated annotation used for pairwise testing.
configure_added_lines = added_lines(target_branch, ["configure.ac"])
switches_added = lines_containing(
configure_added_lines, "AC_ARG_ENABLE"
) + lines_containing(configure_added_lines, "AC_ARG_WITH")
annotations_added = lines_containing(configure_added_lines, "# [pairwise: ")
if switches_added:
if len(switches_added) > len(annotations_added):
fail(
"This merge request adds at least one new `./configure` switch that "
"is not annotated for pairwise testing purposes."
)
else:
message(
"**Before merging**, please start a full CI pipeline for this "
"branch with the `PAIRWISE_TESTING` variable set to any "
"non-empty value (e.g. `1`). This will cause the `pairwise` "
"job to exercise the new `./configure` switches."
)
###############################################################################
# PRE-RELEASE TESTING
###############################################################################
#
# WARN if the merge request is marked with the "Security" label, but not with
# the label used for marking merge requests for pre-release testing (if the
# latter is defined by the relevant environment variable).
pre_release_testing_label = os.getenv("PRE_RELEASE_TESTING_LABEL")
if (
pre_release_testing_label
and "Security" in mr_labels
and pre_release_testing_label not in mr_labels
):
warn(
"This merge request is marked with the *Security* label, but it is not "
f"marked for pre-release testing (*{pre_release_testing_label}*)."
)
###############################################################################
# USER-VISIBLE LOG LEVELS
###############################################################################
#
# WARN if the merge request adds new user-visible log messages (INFO or above)
user_visible_log_levels = [
"ISC_LOG_INFO",
"ISC_LOG_NOTICE",
"ISC_LOG_WARNING",
"ISC_LOG_ERROR",
"ISC_LOG_CRITICAL",
]
source_added_lines = added_lines(target_branch, ["*.[ch]"])
for log_level in user_visible_log_levels:
if lines_containing(source_added_lines, log_level):
warn(
"This merge request adds new user-visible log messages with "
"level INFO or above. Please double-check log levels and make "
"sure none of the messages added is a leftover debug message."
)
break
###############################################################################
# SYSTEM TEST FILES
###############################################################################
#
# FAIL if newly added system test directory contains an underscore (invalid char)
# FAIL if there are no pytest files in the system test directory
# FAIL if the pytest glue file for tests.sh is missing
TESTNAME_CANDIDATE_RE = re.compile(r"bin/tests/system/([^/]+)")
testnames = set()
for path in affected_files:
match = TESTNAME_CANDIDATE_RE.search(path)
if match is not None:
testnames.add(match.groups()[0])
for testname in testnames:
dirpath = f"bin/tests/system/{testname}"
if (
not os.path.isdir(dirpath)
or testname.startswith(".")
or testname.startswith("_")
or testname == "isctest"
):
continue
if "_" in testname:
fail(
f"System test directory `{testname}` may not contain an underscore, "
"use hyphen instead."
)
if not glob.glob(f"{dirpath}/**/tests_*.py", recursive=True):
fail(
f"System test directory `{testname}` doesn't contain any "
"`tests_*.py` pytest file."
)
tests_sh_exists = os.path.exists(f"{dirpath}/tests.sh")
glue_file_name = f"tests_sh_{testname.replace('-', '_')}.py"
tests_sh_py_exists = os.path.exists(f"{dirpath}/{glue_file_name}")
if tests_sh_exists and not tests_sh_py_exists:
fail(
f"System test directory `{testname}` is missing the "
f"`{glue_file_name}` pytest glue file."
)