ci: add script to check for .changelog file in PRs#9641
Conversation
github-actions
Bot
added
the
type/ci
dnephin
left a comment
dnephin
left a comment
There was a problem hiding this comment.
Nice! Thank you for working on this!
One suggestion for allowing this to work with fork PRs from external contributors.
There was a problem hiding this comment.
I guess this means that the check will only work for internal contributions, is that right?
I think it is equally important to have this check run on PRs from external contributors so that we don't have to ask them every time to add a changelog entry.
We should be able to accomplish that by using github actions to do this work instead of circleCI. If we use pull_request_target (https://docs.github.com/en/actions/reference/events-that-trigger-workflows#pull_request_target), which will run using the code from the master branch.
This should allow us to run the check for external contributions as well.
There was a problem hiding this comment.
I think by using github actions we can get this information without an API call by using the github context github.event.pull_request.base.ref and github.event.pull_request.labels.
There was a problem hiding this comment.
I guess using github actions with pull_request_target would require a git checkout to get this list of changed files, since the available code will be of the target branch (master).
|
@dnephin Thanks for the review! Yes after looking over this, doing all the GitHub API calls to get the various pieces I need seems cumbersome. I also realized Will pivot to using GitHub actions to implement this logic instead. That should make the conditionals cleaner. |
alvin-huang
force-pushed
the
ci/changelog-check
branch
from
3be7974 to
c6fe20b
Compare
alvin-huang
force-pushed
the
ci/changelog-check
branch
from
c6fe20b to
21586c7
Compare
alvin-huang
force-pushed
the
ci/changelog-check
branch
from
21586c7 to
f4d95c6
Compare
dnephin
left a comment
dnephin
left a comment
There was a problem hiding this comment.
LGTM, couple small suggestions on wording
| if [ -z "$changelog_files" ]; then | ||
| # post PR comment to GitHub when no .changelog entry was found on PR | ||
| echo "changelog-check: Did not find a .changelog entry, posting a reminder in the PR" | ||
| github_message="🤔 Double check that this PR does not require a changelog entry in the .changelog directory. [Reference](https://github.com/hashicorp/consul/pull/8387)" |
There was a problem hiding this comment.
In the future I guess we should try and document this under contributing/ and link to that instead. But since we don't have those docs yet, this seems good.
There was a problem hiding this comment.
Agreed, I'll keep this as unresolved so it's easier to find if we ever look at this PR again.
| # here: https://securitylab.github.com/research/github-actions-preventing-pwn-requests. But | ||
| # due to this workflow only running a git diff check and not building or publishing anything, | ||
| # there is no harm in checking out the PR HEAD code. |
There was a problem hiding this comment.
I think the most likely risk here would be someone moving this logic into a script, and calling the script from this workflow.
I think it might help to clarify this point with something like this:
All the code checked out in this workflow should be considered untrusted. This workflow must never call any makefiles or scripts. It must never be changed to run any code from the checkout.
There was a problem hiding this comment.
That's true. The only thing that gets 'written' back is a PR comment but the copied script on a PR wouldn't have access to the github token to do that.
That blurb is nice to have though, I'll add it!
Co-authored-by: Daniel Nephin <dnephin@hashicorp.com>
|
🍒 If backport labels were added before merging, cherry-picking will start automatically. To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/321216. |
3 participants
#8387 added go-changelog which generates changelog entries from the
.changelogdirectory. This PR adds a small CI check to add a comment to any PR that does not have thepr/no-changeloglabel to remind the author to double check that the PR does not require a changelog entry.