-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: bundle latest k6 on build #357
Conversation
.github/workflows/release.yml
Outdated
@@ -18,6 +18,43 @@ jobs: | |||
steps: | |||
- uses: actions/checkout@v4 | |||
|
|||
# get k6 binaries macos | |||
- name: get latest k6 binary macos |
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.
Can we run the install-k6
script here so that the code isn't duplicated?
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 suggestion!
I built them in the opposite order so I didn't think about it, but if it works in the action and both locally for windows as well we might be able to just use the single script 👀
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 agree with running node install-k6.js
instead to remove duplication.
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.
We need some adjustments on Windows but it's looking good so far!
getWindowsK6Binary() | ||
console.log('k6 binary download completed') |
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.
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.
Noticed this one as well!
I'm now making use of execSync
instead of exec
and that fixes the message issue as well 🙌
install-k6.js
Outdated
|
||
# move to resource folder | ||
Move-Item -Path "${K6_PATH_WIN_AMD}\\k6.exe" -Destination resources\\win\\x86_64 | ||
` |
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 noticed the temp files were still there after the download was complete (they were also showing up in git status
). We can add a clean up function here similar to what we have for macOS
` | |
# clean up | |
del k6-windows-amd64.zip | |
Remove-Item -Recurse -Force ${K6_PATH_WIN_AMD} | |
` |
install-k6.js
Outdated
const K6_PATH_WIN_AMD = `k6-${K6_VERSION}-windows-amd64` | ||
|
||
const existsK6 = (os, arch) => { | ||
return existsSync(`resources/${os}/${arch}/k6`) |
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 isn't working on Windows because of the missing extension (.exe
) 😞 We may have to do the os check inside this function instead.
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.
We can remove this function now that we are using existsSync
directly.
.github/workflows/release.yml
Outdated
@@ -18,6 +18,43 @@ jobs: | |||
steps: | |||
- uses: actions/checkout@v4 | |||
|
|||
# get k6 binaries macos | |||
- name: get latest k6 binary macos |
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 agree with running node install-k6.js
instead to remove duplication.
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.
It looks good on both Mac and Windows 🚀 Just one minor thing left.
install-k6.js
Outdated
const K6_PATH_WIN_AMD = `k6-${K6_VERSION}-windows-amd64` | ||
|
||
const existsK6 = (os, arch) => { | ||
return existsSync(`resources/${os}/${arch}/k6`) |
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.
We can remove this function now that we are using existsSync
directly.
Description
The binaries for
k6
have been removed and now the specified version ofk6
is retrieved from the k6 github repository during the release ci process. The version is specified by a repository variable calledK6_VERSION
.Due to this, the binary is also missing in
dev
so thenpm install
has been augmented with a script to retrieve the binary if it's missing.note: The version for
dev
is specified inside of theinstall-k6.js
file at this time, so the version is specified in 2 places.How to Test
npm install
making sure you havek6
installed after it.Checklist
npm run lint
) and all checks pass.npm test
) and all tests pass.Screenshots (if appropriate):
Related PR(s)/Issue(s)
Closes #327