From 97364f551805210c4938faf6cd531fce1e3e0a81 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 1/8] Flag missing CVE identifiers 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. --- dangerfile.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dangerfile.py b/dangerfile.py index 1c9177d7b4..9e34b33dc5 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -25,6 +25,9 @@ def added_lines(target_branch, paths): 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] + issue_or_mr_id_regex = re.compile(br'\[(GL [#!]|RT #)[0-9]+\]') release_notes_regex = re.compile(r'doc/(arm|notes)/notes-.*\.(rst|xml)') @@ -197,3 +200,21 @@ if release_notes_changed: identifiers_found = filter(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.') From 2f77c7680a14beb89b7d38007a604e38e21b1521 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 2/8] Handle [placeholder] CHANGES entries 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. --- dangerfile.py | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index 9e34b33dc5..7f07280302 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -149,11 +149,14 @@ elif 'LGTM (Merge OK)' not in mr_labels: # 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 the the "No CHANGES" label is used in +# label set. (This attempts to ensure that the "No CHANGES" label is used in # a sane way.) # -# * The merge request adds a new CHANGES entry that does not contain any -# GitLab/RT issue/MR identifiers. +# * 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 no_changes_label_set = 'No CHANGES' in mr_labels @@ -165,9 +168,15 @@ if changes_modified and no_changes_label_set: 'Revert `CHANGES` modifications or unset the *No Changes* label.') changes_added_lines = added_lines(target_branch, ['CHANGES']) +placeholders_added = lines_containing(changes_added_lines, '[placeholder]') identifiers_found = filter(issue_or_mr_id_regex.search, changes_added_lines) -if changes_added_lines and not any(identifiers_found): - fail('No valid issue/MR identifiers found in added `CHANGES` entries.') +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 From d81ad454cc6aef7f7cf3435847898ae17d362cab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 3/8] Suggest adding release notes for customer issues Make Danger suggest adding a release note to a merge request if the latter is marked with the "Customer" label but not with the "Release Notes" label. --- dangerfile.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index 7f07280302..df1ae1a8e1 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -191,15 +191,26 @@ if changes_added_lines: # Notes" label set. (This ensures that merge requests updating release # notes can be easily found using the "Release Notes" label.) # -# - WARN if 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. +# - 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, modified_files)) release_notes_label_set = 'Release Notes' in mr_labels -if not release_notes_changed and release_notes_label_set: - fail('This merge request has the *Release Notes* label set. ' - 'Add a release note or unset the *Release Notes* label.') +if not release_notes_changed: + if release_notes_label_set: + fail('This merge request has the *Release Notes* label set. ' + 'Add a release note or unset the *Release Notes* label.') + elif 'Customer' in mr_labels: + warn('This merge request has the *Customer* label set. ' + 'Add a release note unless the changes introduced are trivial.') 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.') From ff58ec8cefcb7ccf5b9a37b2f4763bb610f7cc64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 4/8] Flag missing pairwise testing markers 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. --- dangerfile.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/dangerfile.py b/dangerfile.py index df1ae1a8e1..7cec7162cc 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -238,3 +238,18 @@ if lines_containing(changes_added_lines, '[security]'): 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 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.') From 953c810f4161f62c36fc463ce973ad8237e74b5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 5/8] Flag trailing dots in commit subject lines Make the Danger GitLab CI job fail when the subject line for any commit belonging to a merge request contains a trailing dot. --- dangerfile.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dangerfile.py b/dangerfile.py index 7cec7162cc..ad40478f77 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -43,6 +43,8 @@ target_branch = danger.gitlab.mr.target_branch # # * The subject line starts with "fixup!" or "Apply suggestion". # +# * 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: @@ -70,6 +72,8 @@ for commit in danger.git.commits: 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 From 801d13f62fe60436db16f525231172145f8ccde5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 6/8] Only warn about fixup commits once per run 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. --- dangerfile.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/dangerfile.py b/dangerfile.py index ad40478f77..68daa7876c 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -61,12 +61,16 @@ target_branch = danger.gitlab.mr.target_branch # four spaces (useful for pasting compiler warnings, static analyzer # messages, log lines, etc.) +fixup_error_logged = False for commit in danger.git.commits: message_lines = commit.message.splitlines() subject = message_lines[0] - if subject.startswith('fixup!') or subject.startswith('Apply suggestion'): + if (not fixup_error_logged and + (subject.startswith('fixup!') or + subject.startswith('Apply suggestion'))): fail('Fixup commits are still present in this merge request. ' 'Please squash them before merging.') + fixup_error_logged = True if len(subject) > 72: warn( f'Subject line for commit {commit.sha} is too long: ' From 09964e8085494bffb4f8038be30babfaafe66ea3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 7/8] Skip length check for lines containing references 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. --- dangerfile.py | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index 68daa7876c..1943df62b1 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -51,15 +51,21 @@ target_branch = danger.gitlab.mr.target_branch # # * The length of the subject line 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". +# * 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.) +# 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. fixup_error_logged = False for commit in danger.git.commits: @@ -86,7 +92,9 @@ for commit in danger.git.commits: ' release note' 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(' '): + 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).' From bc42690c99d0df39b20510a70d584da6c7c2ac2a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20K=C4=99pie=C5=84?= Date: Mon, 18 Jan 2021 14:57:47 +0100 Subject: [PATCH 8/8] Skip subject line length check for merge commits 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.) --- dangerfile.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dangerfile.py b/dangerfile.py index 1943df62b1..f9db5bd7d7 100644 --- a/dangerfile.py +++ b/dangerfile.py @@ -49,7 +49,8 @@ target_branch = danger.gitlab.mr.target_branch # # - WARN if any of the following is true for any commit on the MR branch: # -# * The length of the subject line exceeds 72 characters. +# * 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: @@ -77,7 +78,7 @@ for commit in danger.git.commits: fail('Fixup commits are still present in this merge request. ' 'Please squash them before merging.') fixup_error_logged = True - if len(subject) > 72: + if len(subject) > 72 and not subject.startswith('Merge branch '): warn( f'Subject line for commit {commit.sha} is too long: ' f'```{subject}``` ({len(subject)} > 72 characters).'