-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Tooling] fix: changelog verification workflow #468
Conversation
.githooks/pre-receive
Outdated
|
||
BRANCH_CHANGED_FILES=($(git diff --name-only main..HEAD)) | ||
# Usage: bash pre-receive <CHANGED FILES> | ||
BRANCH_CHANGED_FILES=($1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the following might be more correct but didn't have time to test it this morning:
BRANCH_CHANGED_FILES=($1) | |
BRANCH_CHANGED_FILES=($@) |
The concern is that only the first file (in the case of $1
) in the space delimited list may be received but I think this may depend on whether the list was quoted or not in the calling context. In contrast $@
will pass all arguments the script receives, in the case where the file list isn't quoted (I think).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"$@"
refers to all arguments "$1
will only refer to arg#1 (first filename)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haha, nice timing @h5law! I just edited my comment - I'm on my way out of the house and had to step away for a sec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we pass "${{needs.changedfiles.outputs.all}}"
what is actually happening is the list of filenames is expanded into this form bash pre-recieve file1 file2 file3 file4 ...
so when we do ($@)
bash is actually taking all these individual file names and storing them into an array we can loop over. 👍🏼
I think it would be worth looking into the reason why the workflow would not find head as the previous solution was cleaner IMO and would remove the need for the changedfiles
workflow aswell.
Co-authored-by: Bryan White <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, I commited the $@
change this refers to all the arguments whereas $1
is only the first.
Description
While recent update to changelog verification works as expected when run locally, it seems to consistently return a false positive result when running in GitHub actions. Testing @h5law's hypothesis (i.e. maybe
HEAD
ormain
aren't what we expect them to be in that environment). TLDR; we probably shouldn't rely on git refs in this context.This PR addresses fixes this false positive scenario as well as cleans up a bit.
Issue
No issue as of time of writing, see: https://discordapp.com/channels/553741558869131266/986789914379186226/1067940783111544952
Type of change
Please mark the relevant option(s):
List of changes
pre-push
conventionally receives 2 arguments (remote and url), which aren't useful for our case; additionally,pre-receive
is a better conceptual fit for the purpose it servesTesting
make develop_test
README
Required Checklist
If Applicable Checklist
shared/docs/*
if I updatedshared/*
README(s)