-
Notifications
You must be signed in to change notification settings - Fork 44
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
fix: ensure Terraform is available for integration tests #390
Conversation
# We're using the latest version of Bun for now, but it might be worth | ||
# reconsidering. They've pushed breaking changes in patch releases | ||
# that have broken our CI. | ||
# Our PR where issues started to pop up: https://github.com/coder/modules/pull/383 | ||
# The Bun PR that broke things: https://github.com/oven-sh/bun/pull/16067 |
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.
Wanted to get this added to the file so that there's a paper trail in case Bun's changes suddenly cause CI to break again
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.
LGTM
Merging to unblock a few community PRs. |
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.
Post-merge 👍
Closes #389
Changes made
process.env
explicitly, and pass it to our child processesNotes
spawn
function, while also (1) pushing it out in a patch release, and (2) making no mention of the changes in their changelog. I'm not sure if we want to tighten up our Bun versioning going forward, but we can always do that in a later PR