-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
[PR to mk/tests] CI iteration #1961
Conversation
@nathanhammond is attempting to deploy a commit to the Vercel Team on Vercel. A member of the Team first needs to authorize 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.
Looks great! one of the reasons I chose not to separate Go/Node is that it started to feel too similar to just actions/setup-go@v3
, et al. I figured the benefit of sharing in repo was that our "abstraction" didn't need to be so clean, and based more on turborepo rather than the underlying tools.
Anywho, this is great and a welcome improvement.
|
||
- name: Build | ||
run: cd cli && make turbo | ||
|
||
- name: Install dependencies | ||
run: pnpm install --filter=benchmark |
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.
so full pnpm 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.
Yeah, low cost since it'll come from local cache.
|
||
- uses: ./.github/actions/setup-deps | ||
with: | ||
go-version: 1.17 |
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.
remove this? as mentioned in #1904 (comment)
- uses: ./.github/actions/setup-deps | ||
- uses: ./.github/actions/setup-node | ||
with: | ||
enable-corepack: false |
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.
+1. This was one of the reasons to split up Node and Go for me too!
Merging this in to see how it behaves, thank you! |
@@ -17,7 +17,7 @@ jobs: | |||
os: [ubuntu-latest, macos-latest] | |||
|
|||
steps: | |||
- uses: actions/checkout@v | |||
- uses: actions/checkout@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.
Good catch! 😅
@mehulkar some of my comments on #1904 are addressed here.