-
Notifications
You must be signed in to change notification settings - Fork 1
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
Create test
and lint
Workflows
#8
Conversation
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.
Great start; thanks for looking into this! One question on setup-node
; a few other changes requested to make sure this applies how we want it to.
.github/workflows/lint.workflow.yml
Outdated
on: | ||
push: | ||
branches: | ||
- '*' |
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.
[Please Change]: For right now, we only want to run this on branches in PRs. It looks like there's an branches-ignore
option that can be used instead for this, which should exclude the following:
master
: There's probably no harm in it; but long-term it'd be nice to have a autodeploy action which will check forlint
/test
there.gh-pages
: This branch has our deployed application (which is not a node project) so I'm guessing the action would fail to run here as well.
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.
Switched from branches
to branches-ignore
in order to exclude master and gh-pages branches 😎
|
||
steps: | ||
- name: Checkout repo | ||
uses: actions/checkout@v2 |
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.
[Question]: Vaguely familiar with GitHub actions. I was expecting to see something like setup-node
here; is it not required? What happens without it?
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.
You're totally right, sorry for the oversight. I've gone ahead and added it in this latest commit /w Node 18 (LTS). Let me know if you think that's good 😄.
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.
LTS is good. FYI; if our project needed node
itself we'd have it specified in package.json
engines
(and we'd want to match it to that) but since we don't this is first pick and LTS is the natural choice.
.github/workflows/lint.workflow.yml
Outdated
uses: actions/checkout@v2 | ||
|
||
- name: Install dependencies | ||
run: npm install |
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.
[Please Change]: Use npm ci
(clean-install
) instead - this verifies the versions used are those in package-lock.json
and will fail if there's any discrepancies (rather than attempting to overwrite with new versions).
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.
Both workflows should now be using npm ci
for installing dependencies :D.
.github/workflows/lint.workflow.yml
Outdated
run: npm install | ||
|
||
- name: Run lint | ||
run: npm run lint |
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.
[Nit]: Missing newline.
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.
Just added a newline at the end of both files 👍.
@@ -0,0 +1,17 @@ | |||
on: |
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.
[Please Change]: Most of the notes above apply here too. For disabling this until we have npm run test
setup; I think we can do that through the GitHub UI and that's fine for this.
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- name: Set up Node |
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.
[Comment]: Should be after Checkout repo
I believe.
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.
Went ahead and fixed that in this latest commit 👍
.github/workflows/lint.workflow.yml
Outdated
|
||
- name: Run lint | ||
run: npm run lint | ||
|
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.
Whoops; this has an extra space so the newline isn't applying.
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.
Thank you for catching that, I've gone ahead and remove the extra spaces at the end of that.
This PR adds two workflows, one for linting and one for testing. Testing workflow should be disabled until tests are added, otherwise it will yield an error due to the lack of a
test
script inpackage.json