-
Notifications
You must be signed in to change notification settings - Fork 4
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
Switch to suffolklitlab org, allow testing pre-release, fix #527
Conversation
typo in regex branch name parsing. Close #393 for v4. Will not be addressing v3.
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.
LGTM! My GitHub Actions-fu isn't great, but from what I know this looks good 👍
with: | ||
node-version: "17" | ||
- run: npm install | ||
shell: bash |
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.
illegitimate nitpick: it is out-of-scope of this PR, but I believe https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#defaultsrun will remove the need of repeating the shell: bash
.
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.
Co-authored-by: Jonatan Asketorp <2598631+k3KAW8Pnf7mkmdSMPHz27@users.noreply.github.com>
|
||
# Install node | ||
- name: "ALKiln: Install node packages" | ||
uses: actions/setup-node@v1 |
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.
The latest setup-node is version 2
It makes sense that Dependabot does this by default, but if it doesn't it can be configured to do so as well (e.g., https://github.com/codeforamerica/brigade-project-index-statusboard/blob/a288e657777479ea82ce0fa09a056ef956a83ce3/.github/dependabot.yml#L3-L6)
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.
branch_name = process.env.BRANCH_PATH.replace( `refs/head/`, '' ); | ||
branch_name = process.env.BRANCH_PATH.replace( `refs/heads/`, '' ); |
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.
Also out of scope of this PR, but the github.ref
variable in the action can be from tags as well. DA doesn't let you pull from tags, just a note that should make it into docs or comments of the action.yml at some point.
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.
Thanks! Co-authored-by: Bryce Willey <Bryce.Steven.Willey@gmail.com>
typo in regex branch name parsing.
Close #393 for v4. Will not be addressing v3.
See it working at https://github.com/plocket/docassemble-CompositeActionsTest/actions/runs/2029985148
https://github.com/plocket/docassemble-CompositeActionsTest/actions/runs/2029915837[This is supposed to be derived from https://github.com/SuffolkLITLab/ALKiln/compare/releases/v4...393_test_action_npm, which is a more tested version. I just didn't want all those experimental commits.]