Skip to content

Conversation

savil
Copy link
Collaborator

@savil savil commented Apr 5, 2023

Summary

RFC: we should have failures in the devbox-scripts reflect in a non-zero exit status
from devbox run <script>.

There is some risk that customers whose code was previously running will now fail. However, their scripts were indeed failing and its just that this failure will now be exposed.

Update: Based on John's feedback in a comment below I am adding a featuregate that is activated in examples tests. We'll make a plan for rolling this out to users, in a way that is not a big surprise for them.

TODO:

  • fix examples which have failures that were previously masked. Done in previous PRs in the stack.

How was it tested?

Removed curl from lepp-stack packages, and ran:

DEVBOX_EXAMPLE_TESTS=1 DEVBOX_DEBUG=0 go test -count 1 -v -run TestExamples/stacks_lepp ./testscripts/...

This failed, demonstrating that the set -e was working as intended.

I then added the curl back and the above command passed.

Copy link
Collaborator Author

savil commented Apr 5, 2023

@savil savil requested review from ipince and Lagoja April 5, 2023 21:22
@savil savil force-pushed the savil/exit-on-failure-scripts branch 2 times, most recently from eff9a6d to 84c2655 Compare April 5, 2023 21:50
@Lagoja
Copy link
Contributor

Lagoja commented Apr 6, 2023

Is this done only for testscripts? or for all devbox scripts?

@savil
Copy link
Collaborator Author

savil commented Apr 6, 2023

@Lagoja this is for all users. Thoughts?

@savil savil changed the base branch from main to savil/lepp-stack-fixes April 7, 2023 01:13
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 84c2655 to a9030e9 Compare April 7, 2023 01:13
@Lagoja
Copy link
Contributor

Lagoja commented Apr 7, 2023

Was thinking about this a bit -- since -e makes a script exit immediately upon an error, I worry that this could cause a big wave of unexpected errors if it's automatically pushed to users.

I think we'd need to either make this something configurable, or do it in a roll out where it's explicitly described and developers have a chance to adapt.

I do think forcing it on our examples and testscripts is a good idea -- we should hold those to a really high standard

@savil savil force-pushed the savil/lepp-stack-fixes branch from bbb30db to 8014da0 Compare April 7, 2023 04:44
@savil savil force-pushed the savil/exit-on-failure-scripts branch from a9030e9 to 0bdf7d2 Compare April 7, 2023 04:44
@savil savil mentioned this pull request Apr 7, 2023
@savil
Copy link
Collaborator Author

savil commented Apr 7, 2023

Sounds good. Let me hold off on this then.

@savil savil closed this Apr 7, 2023
@savil savil reopened this Apr 7, 2023
@savil savil marked this pull request as draft April 7, 2023 18:14
@savil savil removed request for Lagoja and ipince April 7, 2023 18:14
@savil
Copy link
Collaborator Author

savil commented Apr 7, 2023

I'll revive this and gate it for example tests

@savil savil force-pushed the savil/lepp-stack-fixes branch from 77439fb to 8a5de32 Compare April 7, 2023 18:16
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 0bdf7d2 to 9c65831 Compare April 7, 2023 18:16
@savil savil force-pushed the savil/lepp-stack-fixes branch 2 times, most recently from 1f9bf9f to 9e60d9b Compare April 7, 2023 18:57
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 9c65831 to efe3f08 Compare April 7, 2023 19:00
@savil savil force-pushed the savil/lepp-stack-fixes branch from 9e60d9b to a26da0d Compare April 7, 2023 19:10
@savil savil force-pushed the savil/exit-on-failure-scripts branch from efe3f08 to 971e5d4 Compare April 7, 2023 19:10
@savil savil force-pushed the savil/lepp-stack-fixes branch from a26da0d to 71e22eb Compare April 7, 2023 19:14
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 971e5d4 to 4644b71 Compare April 7, 2023 19:14
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 33755df to fad67f5 Compare April 8, 2023 00:25
@savil savil force-pushed the savil/lepp-stack-fixes branch from b60ab66 to ccdce85 Compare April 8, 2023 00:35
@savil savil force-pushed the savil/exit-on-failure-scripts branch from fad67f5 to 7ef9aac Compare April 8, 2023 00:36
@savil savil changed the base branch from savil/lepp-stack-fixes to savil/testscripts-env-functions April 8, 2023 01:06
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 7ef9aac to f7d2bfe Compare April 8, 2023 01:06
@savil savil force-pushed the savil/testscripts-env-functions branch from 7038c73 to ad06e55 Compare April 8, 2023 01:41
@savil savil force-pushed the savil/exit-on-failure-scripts branch from f7d2bfe to 8d325c6 Compare April 8, 2023 01:41
@savil savil force-pushed the savil/testscripts-env-functions branch from ad06e55 to 73ba576 Compare April 10, 2023 22:25
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 8d325c6 to a4264b0 Compare April 10, 2023 22:25
@savil savil force-pushed the savil/testscripts-env-functions branch from 73ba576 to 327692e Compare April 10, 2023 22:37
@savil savil force-pushed the savil/exit-on-failure-scripts branch from a4264b0 to dfed527 Compare April 10, 2023 22:37
@savil savil force-pushed the savil/testscripts-env-functions branch from 327692e to 3c1b417 Compare April 10, 2023 22:42
@savil savil force-pushed the savil/exit-on-failure-scripts branch from dfed527 to 1c644b0 Compare April 10, 2023 22:43
@savil savil force-pushed the savil/testscripts-env-functions branch from 3c1b417 to 069ec79 Compare April 10, 2023 22:44
@savil savil force-pushed the savil/exit-on-failure-scripts branch 2 times, most recently from 5bf91ba to f3ef8b3 Compare April 10, 2023 23:43
@savil savil requested review from gcurtis, mikeland73 and Lagoja April 11, 2023 00:05
@savil savil marked this pull request as ready for review April 11, 2023 00:05
@savil savil force-pushed the savil/testscripts-env-functions branch from 1f39049 to 3397c47 Compare April 11, 2023 23:28
@savil savil force-pushed the savil/exit-on-failure-scripts branch from f3ef8b3 to f3e2ea1 Compare April 11, 2023 23:28
@savil savil force-pushed the savil/testscripts-env-functions branch from 3397c47 to 3d43a84 Compare April 11, 2023 23:37
@savil savil force-pushed the savil/exit-on-failure-scripts branch from f3e2ea1 to 0010997 Compare April 11, 2023 23:37
Base automatically changed from savil/testscripts-env-functions to main April 12, 2023 00:03
@savil savil force-pushed the savil/exit-on-failure-scripts branch from 0010997 to d54ce11 Compare April 12, 2023 00:04
@savil savil merged commit d25a3a5 into main Apr 12, 2023
@savil savil deleted the savil/exit-on-failure-scripts branch April 12, 2023 00:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants