Skip to content
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

[wrappers] Don't re-source shellenv if already sourced #872

Merged
merged 3 commits into from
Apr 6, 2023

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Apr 6, 2023

Summary

This fixes issue described by https://github.com/amithgeorge/devbox-nodejs-repro-20230406

It's not a perfect fix to all wrapper issues. Specifically:

USER=foo node -e "console.log(process.env.USER);"

will print the shellenv user instead of foo.

Will follow up with a fix for that.

cc: @amithgeorge

How was it tested?

Followed steps in https://github.com/amithgeorge/devbox-nodejs-repro-20230406

Added example that breaks in current release, passes in this PR

Copy link
Contributor

@loreto loreto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Although tempted to move that example to testscripts/ instead ... it feels less like an example where we are teaching users how to use devbox in a particular use case, and more like a test-case designed to catch this edge case.

@mikeland73
Copy link
Contributor Author

Although tempted to move that example to testscripts

@loreto yeah totally. I messed up

@mikeland73 mikeland73 merged commit bec08b4 into main Apr 6, 2023
9 checks passed
@mikeland73 mikeland73 deleted the landau/fix-re-source branch April 6, 2023 23:10
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.

None yet

2 participants