ci: longer timeout, break out lint checks to own yml #382
Conversation
jooohhn
left a comment
jooohhn
left a comment
There was a problem hiding this comment.
Overall LGTM with some clarifications
| name: Test | ||
|
|
||
| on: [push, pull_request] | ||
| # https://github.community/t/how-to-trigger-an-action-on-push-or-pull-request-but-not-both/16662 |
There was a problem hiding this comment.
Would this config stop tests on non-main branches?
There was a problem hiding this comment.
This would stop tests on push for non-main branches, only run when they put a PR on main.
This is also an important callout - if we change our core branch name again this is another piece to maintain. thoughts?
There was a problem hiding this comment.
have we changed the core branch name in the past? Is there a reason to think we might do so again?
There was a problem hiding this comment.
Forgot to respond here!
It changed once from master -> main, but I feel like it'd be pretty stable after that? Just the idea that the head branch can change
| jobs: | ||
| lint-check: | ||
| name: Check for ESLint + Prettier Violations | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Do we want ubuntu-latest over ubuntu-18.04?
There was a problem hiding this comment.
ubuntu-latest is 20, not 18 actually. I changed it here since the environment of the linting probably did not matter as much as the actual unit test suite.
Do you have a preference between either? The reason I made this change for mac/ubuntu is cuz the Node SDK actions gave me a warning when I was running not the latest version when I was shoring up the tests there
.github/workflows/test.yml
Outdated
| matrix: | ||
| node-version: [10.x, 12.x, 14.x] | ||
| os: [macos-10.14, ubuntu-18.04] | ||
| os: [macOS-latest, ubuntu-18.04] |
There was a problem hiding this comment.
ubuntu-18.04 here is mixed with ubuntu-latest in other files
| - name: Build and run tests | ||
| run: | | ||
| make test |
There was a problem hiding this comment.
Are the changes in this block all whitespace?
There was a problem hiding this comment.
I think so - just my yml linter; let me check which one i'm using
|
@jooohhn @ajhorst I've gone an updated them all to ubuntu-latest, which will keep us up-to-date. Only concern here is if the latest has some critical error that causes stuff to break, but these are unit tests and not mission-critical if we get some unexpected failure; we can probably deal with that if we (ever) come to it? |
Summary
Checklist