chore: improve workflow #455
Conversation
🦋 Changeset detectedLatest commit: 248f926 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| push: | ||
| branches: ["main"] | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, ready_for_review] |
There was a problem hiding this comment.
Why narrow this job down to these types?
There was a problem hiding this comment.
The idea was to have the CI auto-run for every PR to the repo and this would prevent the CI to run on PRs that didn't meet these criteria. I know we discarded the idea of having it auto-run on every third-party PR, but I think this is still good, wdyt?
There was a problem hiding this comment.
What's wrong with having it run on every PR?
There was a problem hiding this comment.
The initial plan for this was that if we would change the permissions so that we wouldn't need to approve a workflow for it to run, so by doing these we would make sure only these types of PR would auto-run the workflow
There was a problem hiding this comment.
We need to approve the workflows for security reasons, I don't think narrowing down the PR type will help. I recommend cutting this but it's up to you.
.github/workflows/validate.yaml
Outdated
| - name: setup-node | ||
| uses: actions/setup-node@v3 | ||
| with: | ||
| node-version: 20 |
There was a problem hiding this comment.
Just curious, what happens if you remove this? I think the ubuntu image already has node. That's why the CI job doesn't include this step and why the yarn install step before it works.
There was a problem hiding this comment.
So doing a bit more digging, yes it does work but apparently the reason people still include this is that they don't want to rely on the default that comes installed with ubuntu-latest and have a bit more of a controlled environment. Other than that it also manages the Node.js installation in a way that is more optimized for Github Actions.
There was a problem hiding this comment.
I understand but we should be consistent. Either use it in all the workflows or none. And if you're going to add it, use actions/setup-node@v4 which is the latest.
There was a problem hiding this comment.
Agreed here, I will look into adding it to all the other workflows on another PR, we can discuss this further
| @@ -0,0 +1,24 @@ | |||
| import { getFilePaths } from './utils.js'; | |||
There was a problem hiding this comment.
FYI it doesn't have to be in this PR but this file would be a good place to solve this: #209
There was a problem hiding this comment.
Sounds good, we can discuss this later, but I was maybe thinking having a different file for checks related to chain and checks related to deployments
| push: | ||
| branches: ["main"] | ||
| pull_request: | ||
| types: [opened, synchronize, reopened, ready_for_review] |
There was a problem hiding this comment.
We need to approve the workflows for security reasons, I don't think narrowing down the PR type will help. I recommend cutting this but it's up to you.
Description
Improve the workflow a bit with separating validation and optimization:
PD: still need to change optimization workflow to run on (probably) every commit to main
Backward compatibility
Yes
Testing
Manual