Repository navigation
Conversation
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
Coverage Report for CI Build 38902399597Coverage decreased (-0.01%) to 74.744%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats💛 - Coveralls |
Signed-off-by: laim2003 <luk.faber@gmx.de>
Signed-off-by: laim2003 <luk.faber@gmx.de>
…nternal-label-Changelog-handling # Conflicts: # .github/workflows/check-pr-labels.yml
Signed-off-by: laim2003 <luk.faber@gmx.de>
…nternal-label-Changelog-handling
quando632
left a comment
There was a problem hiding this comment.
Nice extension of the label check. Re-fetching the PR so the label state is current, paginating listFiles, and keeping the workflow at pull-requests: read are all solid choices. The failure messages tell the contributor exactly which two ways lead to a green check and point to the DoD, which makes the rule easy to follow.
| + `Please remove the entry from ${CHANGELOG}, or remove the "${INTERNAL_LABEL}" ` | ||
| + `label if the change is user-facing. See documentation/contributing/DoD.adoc.` | ||
| ); | ||
| } else if (!isInternal && addedLines === 0) { |
There was a problem hiding this comment.
This branch fails for PRs that the DoD explicitly exempts from the CHANGELOG, namely follow-up fixes to a story that has not been released yet (DoD.adoc, line 36). The same applies to test-only, workflow and docs PRs without the internal label: simulating this rule on the last 100 merged PRs, 17 non-Dependabot PRs would have failed (e.g. #2383, #2435, #2350, #2564). Could we update that DoD paragraph to say "label such PRs internal", so contributors following the DoD do not end up with a red check?
| + `Please remove the entry from ${CHANGELOG}, or remove the "${INTERNAL_LABEL}" ` | ||
| + `label if the change is user-facing. See documentation/contributing/DoD.adoc.` | ||
| ); | ||
| } else if (!isInternal && addedLines === 0) { |
There was a problem hiding this comment.
Dependabot PRs will always fail here, since they only get dependencies/java labels and never touch the CHANGELOG (#2259, #2260, #2339, #2340 would all have failed). Adding labels to the maven entry in .github/dependabot.yml would fix that; explicit labels replace Dependabot's defaults, so dependencies needs to be listed too:
labels:
- "dependencies"
- "internal"
shodiBoy1
left a comment
There was a problem hiding this comment.
Thanks @laim2003 for the work on this.
The PR does what the issue asks.
One thing to keep in mind: the current code treats every added line as a changelog entry. For example, adding an empty line or changing whitespace makes the check pass without internal, while the same change makes it fail with internal.
This is not blocking from my side because we still review the changelog changes manually. We can improve the check later if needed.
This PR fixes #2647
Depends on #2645
Implemented changes:
Testing instructions
Please add conscise, understandable instructions on how a reviewer can test/verify the functionality of your contribution here:
Checklist for this PR
Make sure everything is checked before merging this PR. For further info please also see
our DoD.
mvn clean testlocally all tests pass and build is successful#«issue-id»: «brief summary»(e.g.#921: fixed setup.batand notfeature/921 fixed setup.bat).If no issue ID exists, title only.
In Progressand assigned to you or there is no issue (might happen for very small PRs)with
internalpom.xmlfiles or otherwise if runtime dependencies changed,you have updated our LICENSE.adoc