The logic contained in dangerfile.py incorrectly warns about missing
release note changes for merge requests preparing release documentation
as such merge requests rename files in the doc/notes/ directory. This
(correctly) causes these files to be passed to dangerfile.py via
danger.git.created_files and danger.git.deleted_files rather than via
danger.git.modified_files, which in turn causes the logic checking the
use of the "Release Notes" label to assume that no release notes are
added, removed, or modified by a given merge request.
Fix by considering all types of file changes (modifications, additions,
and removals - which also covers file renaming) when checking whether a
given merge request modifies release notes. Update the warning messages
accordingly.
However, when trying to find release notes added by a given merge
request, deleted files must not be considered. Tweak the logic looking
for GitLab identifiers in the release notes added by a given merge
request so that it only scans modified and added (or renamed) files.
A merge request might have no description at all (i.e. None, rather than
an empty string). This might happen when the MR is created via an API.
Check a description is present before trying to find a backport string
in it.
Since the list of lines added to Git-tracked text files in a given
branch is not part of the Danger DSL [1], it is determined using custom
code in dangerfile.py. The current implementation of that logic is less
than perfect as it examines the diff between the current tip of the
target branch and the source branch rather than the diff between the
merge base of the two branches and the source branch. Consider a Git
history like this:
* F (target)
...
* E
* D
* C
| * B (source)
|/
* A (merge base)
If danger-python or Hazard are run for commit B, the current logic for
determining the list of added lines in dangerfile.py examines the diff
between commits F and B rather than between commits A and B. Therefore,
the added_lines() function returns not just the lines added by commit B
on top of commit A, but also the list of lines that were removed between
commits A and F, which leads to confusing results.
Fix by using the triple-dot diff operator in the Git invocation whose
output is used as the source of information for determining the list of
lines added by a given branch.
Since Hazard fetches the target branch itself when it is run, remove the
explicit "git fetch" invocation that fetches the target branch from
GitLab (shortening its local history to a single commit in the process)
before "git diff" is invoked.
[1] https://danger.systems/js/reference.html#GitDSL
Create a utility package for code shared by the python tests. The
utility functions should use reasonable defaults and be split up into
modules according to their functionality.
Ensure assert rewriting is enabled for the modules to get the most
useful output from pytest.
When adding a new system test, it might easy to forget to add the
required files for the pytest runner or break a naming convention. Add
danger checks to cover these cases.
The fixup commits' subject line has a prefix which has its own
length, so warning about the exceeding length is not accurate.
Given that the fixup commits can not be merged, because they
cause a danger failure, it's safe to ignore the length check
for them.
The following are not also accepted as single-line commits without
generating warnings:
- CHANGES/release note may appear in the beginning of the commit message
- Release note may be capitalized
- Allow commits with "GL #" (e.g. Update documentation for [GL #XXXX])
commit --fixup=amend:<hash> produces a subject starting with amend!
by default. Have danger look for this to ensure that it is squashed
before merging.
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.
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.
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
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]).
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).
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.
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.
Messages with log levels INFO or higher are flagged for manual review.
Purpose of this check is to prevent debug logs to being released with
too-high log level.
This commit converts the license handling to adhere to the REUSE
specification. It specifically:
1. Adds used licnses to LICENSES/ directory
2. Add "isc" template for adding the copyright boilerplate
3. Changes all source files to include copyright and SPDX license
header, this includes all the C sources, documentation, zone files,
configuration files. There are notes in the doc/dev/copyrights file
on how to add correct headers to the new files.
4. Handle the rest that can't be modified via .reuse/dep5 file. The
binary (or otherwise unmodifiable) files could have license places
next to them in <foo>.license file, but this would lead to cluttered
repository and most of the files handled in the .reuse/dep5 file are
system test files.
Some merge requests (e.g. those created for release branches) include
merge commits. Prevent Danger from warning about excessive subject line
length for merge commits. (While the proper way to detect a merge
commit would be to check the 'parents' attribute of a commit object,
Danger Python does not seem to populate that attribute, so a simple
string search is performed on the commit subject instead.)
The Danger GitLab CI job currently flags excessively long lines in
commit log messages. Exclude lines containing references (i.e. starting
with "[1]", "[2]", etc.) from this check. This allows e.g. long URLs to
be included in commit log messages without triggering Danger warnings.
The Danger GitLab CI job currently generates a separate error message
about fixup commits being present in a merge request for every such
commit found. Prevent that by making it only log that error message
once per run.
Make the Danger GitLab CI job fail when a merge request adds a new
./configure switch without also adding a "# [pairwise: ...]" marker that
the relevant GitLab CI job uses for preparing the pairwise testing
model. This helps to ensure that any newly added ./configure switches
are tested by the pairwise testing GitLab CI job.
Make the Danger GitLab CI job fail when a merge request targeting a
branch different than "main" adds any [placeholder] entries to the
CHANGES file. Prevent Danger from flagging missing GitLab identifiers
for [placeholder] CHANGES entries.
Make Danger ensure that if a merge request fixes a security issue then
that merge request includes a CHANGES entry and a release note, both of
which contain a CVE identifier.
The Danger script inspects differences between the current version of a
given merge request's target branch and the merge request branch. If
the latter falls behind the former, the Danger script will wrongly warn
about missing GitLab/RT identifiers because it incorrectly treats the
"+++" diff marker as an indication of the merge request adding new lines
to a file. Tweak the relevant conditional expression to prevent such
invalid warnings from being raised.
As GitLab Runner Docker executor caches Git repositories between jobs,
prevent the Danger script from attempting to update local refs to ensure
"git fetch" returns with an exit code of 0. Use the FETCH_HEAD ref for
determining the differences between the merge request branch and its
target branch.
Commits adding CHANGES entries and/or release notes do not need a commit
log message. Do not warn about a missing commit log message for such
commits to make the warning more meaningful.
Certain rules of the BIND development process are not codified anywhere
and/or are used inconsistently. In an attempt to improve this
situation, add a GitLab CI job which uses Danger Python to add comments
to merge requests when certain expectations are not met. Two categories
of feedback are used, only one of which - fail() - causes the GitLab CI
job to fail. Exclude dangerfile.py from Python QA checks as the way the
contents of that file are evaluated triggers a lot of Flake8 and PyLint
warnings.
