Skip to content
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

Merged
merged 4 commits into from
Jan 27, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 2 additions & 23 deletions .githooks/pre-push → .githooks/pre-receive
Original file line number Diff line number Diff line change
@@ -1,28 +1,7 @@
#!/usr/bin/env bash

# An example hook script to verify what is about to be pushed. Called by "git
# push" after it has checked the remote status, but before anything has been
# pushed. If this script exits with a non-zero status nothing will be pushed.
#
# This hook is called with the following parameters:
#
# $1 -- Name of the remote to which the push is being done
# $2 -- URL to which the push is being done
#
# If pushing without using a named remote those arguments will be equal.
#
# Information about the commits which are being pushed is supplied as lines to
# the standard input in the form:
#
# <local ref> <local oid> <remote ref> <remote oid>
#
# This sample shows how to prevent push of commits where the log message starts
# with "WIP" (work in progress).

remote="$1"
url="$2"

BRANCH_CHANGED_FILES=($(git diff --name-only main..HEAD))
# Usage: bash pre-receive <CHANGED FILES>
BRANCH_CHANGED_FILES=($1)
Copy link
Contributor Author

@bryanchriswhite bryanchriswhite Jan 27, 2023

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:

Suggested change
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).

Copy link
Contributor

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)

Copy link
Contributor Author

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.

Copy link
Contributor

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.


# Initialise arrays and script wide variables
IGNORE_DIRS=(".github" ".githooks" "docs" "bin")
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/changelog-verify.yml
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,4 @@ jobs:
steps:
- uses: actions/checkout@v3
- name: Verify changelogs
run: bash ./.githooks/pre-push ${{needs.changedfiles.outputs.all}}
run: bash ./.githooks/pre-receive "${{needs.changedfiles.outputs.all}}"