-
Notifications
You must be signed in to change notification settings - Fork 1k
refactor: make CI do less unneeded work #11372
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
Conversation
|
14a6d64 to
fc45a63
Compare
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| turbo-token: ${{ secrets.TURBO_TOKEN }} | ||
| turbo-signature: ${{ secrets.TURBO_REMOTE_CACHE_SIGNATURE_KEY }} | ||
|
|
||
| - name: List changed files |
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.
moving this closer to where it is used and only if needed
| - name: Install Dependencies | ||
| uses: ./.github/actions/install-dependencies | ||
| with: | ||
| pnpm-filters: "./packages/*" |
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.
Doesn't this need tools 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.
I only see .github/prereleases/upload.mjs used.
We should probably move this to the tools as a follow up.
It would also be nice to make sure tools declate all of its deps so that we don't have to install the root package, another follow up.
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.
Cool. I'll guess we'll notice sooner rather than later.
Co-authored-by: Pete Bacon Darwin <[email protected]>
petebacondarwin
left a comment
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.
Given that we are changing this. Don't get-changed-files and paths-filter more or less do the same thing? Could we get by only using one of them?
petebacondarwin
left a comment
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.
This works for me.
|
|
Thanks for the review Pete! |
This PR adds a
pnpm-filtersarg to filter what packages are installed in.github/actions/install-dependencies/action.ymlWhen the action is used by some workflow we would only installed the required packages which should save some time/bw.