Repository: scikit-learn/scikit-learn
Pull request: CI Add workflow to check Changelog entry.
#### Reference Issues/PRs
Fixes #10451

#### What does this implement/fix? Explain your changes.
Add a github action checking if the pull request is referenced in the changelog together with its author.

#### Any other comments?
Need the creation of a label "no-changelog" for pull requests that do not need to be listed.
This is the same approach used by astropy.

See its test implementation [here](https://github.com/test4actions/hello-github-actions/pull/5).

Problem statement:
Have Travis ensure that what's new is updated
Sometimes when PRs are merged I waste time going back to double check that what's new has been updated.

We should make a travis run for PRs only that fails if tests are modified and what's new isn't modified. Perhaps we should even check that the diff to what's new mentions the current PR# (although there are sometimes exceptions to this).

Does this seem a reasonable policy?

Hints from issue discussion:
Since that I am the first one to forget about it, it would be awesome (until the point that I want to kill Travis to throw those errors in addition of the flake8 one :P). Indeed, I am not sure 100% which case we are supposed to do a what's new entry or not.

The flake8 build could serve for such checking I think.
I think since it would be very fast, a separate build is worthwhile so we can see a separate tick or x for it... but I'm okay with it being in flake8 build too....
I'm suggesting the relationship changed tests -> what's new under the
assumption that we must report changed behaviours in what's new, and
changed behaviours must be reflected in changed tests (which we're much
better at requiring than what's new).

On 11 January 2018 at 11:29, Guillaume Lemaitre <notifications@github.com>
wrote:

> Since that I am the first one to forget about it, it would be awesome
> (until the point that I want to kill Travis to throw those errors in
> addition of the flake8 one :P). Indeed, I am not sure 100% which case we
> are supposed to do a what's new entry or not.
>
> The flake8 build could serve for such checking I think.
>
> —
> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/scikit-learn/scikit-learn/issues/10451#issuecomment-356784236>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAEz6z9e-atoE5blNfrO1S2cAdUtkDCbks5tJVXygaJpZM4RaJLV>
> .
>

> changed tests -> what's new

+1 
With heavy bias from https://github.com/scikit-learn/scikit-learn/issues/10446, [danger](http://danger.systems/js/) seems to be able to do that.

The slightly annoying thing with having a build failure if you don't add a whats_new entry is that they tend to be added once the PR is ready to merge, because it is easier to write a good summary then. Having a build failure from the beginning would probably lead to write a not so great whats_new entry until the PR is ready to merge. At this point, it is easy to forget to check the whats_new entry ...

yes, I realise that, but think it's worth trying and seeing if having
what's new from the start helps in reviewing and if we get used to editing
it towards the end

what would danger do to notify us of a missing what's new?

As far as I understand, you can write rules about what danger is supposed to check and then the danger bot comments on the issue with some summary (looks like there is only one bot comment at any single time, not sure I have not thoroughly checked).

Here is an example comment from their doc:
![](http://danger.systems/images/dangerjs-screenshot-2e2a9281.png)
shrug. I'd rather just try one or other solution and wait to see what we
think

Commit message: More explicit comment and message

Co-authored-by: Nicolas Hug <contact@nicolas-hug.com>

Patch to review:
diff --git a/.github/workflows/check_changelog.yml b/.github/workflows/check_changelog.yml
new file mode 100644
index 0000000000000..99524298ccad7
--- /dev/null
+++ b/.github/workflows/check_changelog.yml
@@ -0,0 +1,33 @@
+name: Check Changelog
+# This check makes sure that the changelog is properly updated
+# when a PR introduces a change in a test file.
+# To bypass this check, label the PR with  no-changelog.
+on:
+  pull_request:
+
+jobs:
+  check:
+    runs-on: ubuntu-latest
+    if: ${{ contains(github.event.pull_request.labels.*.name, 'no-changelog') == 0 }}
+    steps:
+      - name: Get PR number
+        run: |
+          echo "PR_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV
+      - uses: actions/checkout@v2
+        with:
+          fetch-depth: '0'
+      - name: Check the changelog only if tests have been modified
+        run: |
+          set -xe
+          tests=$(git diff --name-only origin/master)
+          if [[ "$tests" =~ tests ]]
+          then
+            result=$(cat ./doc/whats_new/v*.rst)
+            if [[ "$result" =~ :pr:\`$PR_NUMBER\` ]]
+            then
+              echo "Changelog has been updated."
+            else
+              echo "Changelog entry is missing. If no changelog entry is required for this PR, label the PR with no-changelog to bypass this check."
+              exit 1
+            fi
+          fi


Code review comments:
diff_hunk: @@ -0,0 +1,33 @@
+name: Check Changelog
+# This check makes sure that the changelog is properly updated
+# when a PR introduces a change in a test file.
+# To bypass this check, label the PR with  no-changelog.
+on:
+  pull_request:
+
+jobs:
+  check:
+    runs-on: ubuntu-latest
+    if: ${{ contains(github.event.pull_request.labels.*.name, 'no-changelog') == 0 }}
+    steps:
+      - name: Get PR number
+        run: |
+          echo "PR_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV
+      - uses: actions/checkout@v2
+        with:
+          fetch-depth: '0'
+      - name: Check the changelog only if tests have been modified
+        run: |
+          set -xe
+          tests=$(git diff --name-only origin/master)
+          if [[ "$tests" =~ tests ]]
+          then
+            result=$(cat ./doc/whats_new/v*.rst)
path: .github/workflows/check_changelog.yml
text: @user1:
If the PR is flagged for a particular milestone it would be great to check that the changelog entry is written in the matching changelog file (for the first two components of the version number).

diff_hunk: @@ -0,0 +1,33 @@
+name: Check Changelog
+# This check makes sure that the changelog is properly updated
+# when a PR introduces a change in a test file.
+# To bypass this check, label the PR with  no-changelog.
+on:
+  pull_request:
+
+jobs:
+  check:
+    runs-on: ubuntu-latest
+    if: ${{ contains(github.event.pull_request.labels.*.name, 'no-changelog') == 0 }}
+    steps:
+      - name: Get PR number
+        run: |
+          echo "PR_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV
+      - uses: actions/checkout@v2
+        with:
+          fetch-depth: '0'
+      - name: Check the changelog only if tests have been modified
+        run: |
+          set -xe
+          tests=$(git diff --name-only origin/master)
+          if [[ "$tests" =~ tests ]]
+          then
+            result=$(cat ./doc/whats_new/v*.rst)
+            if [[ "$result" =~ :pr:\`$PR_NUMBER\` ]]
+            then
+              echo "Changelog has been updated."
+            else
+              echo "Changelog entry is missing. If no changelog entry is required for this PR, label the PR with no-changelog to bypass this check."
path: .github/workflows/check_changelog.yml
text: @user1:
Maybe we should be extra-explicit on case where we need a changelog entry:

- bug fixes
- new features (in particular with changes to the public API, e.g. with a new public class or function or a new parameter)

Changes that do not need a changelog entry:

- documentation fixes or improvements
- new or updated examples or benchmark
- fix or improvements in the test suite
- code style changes
- updated continuous integration configuration
- updated packaging configuration or tools (unless the previously released packages on pypi.org or conda-forge where invalid for some reason, in which case it's useful to mention the list of problematic packages in the changelog)

Note that it is ok to reference several PRs in the same changelog entry, for instance when a big task has been decomposed in subtasks or if a partial fix has been further improved in a follow-up PR.

@author:
>     * fix or improvements in the test suite

wrt the current implementation this case will be checked because test files are likely to be modified. Should I get rid of this check?
https://github.com/scikit-learn/scikit-learn/blob/62a506861e9c16348764e5187da6749ac3402312/.github/workflows/check_changelog.yml#L22-L23

> Note that it is ok to reference several PRs in the same changelog entry, for instance when a big task has been decomposed in subtasks or if a partial fix has been further improved in a follow-up PR.

The current implementation check if the submitted PR is cited in the changelog: this is always true when a changelog is needed, right?


@user1:
If the PR is only a about fixing a bad test or adding a new one, we shall not document it in a changelog entry because it's probably useless for our end-users. I mean the test fix is useful to us and our users but since the tests are not part of the public API, it's not that interesting to the readers of the changelog.

> The current implementation check if the submitted PR is cited in the changelog: this is always true when a changelog is needed, right?

Yes, this is correct.