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

fix: Set default shell version values on POSIX entrypoint #1594

Merged
merged 2 commits into from
Sep 10, 2023

Conversation

kurochan
Copy link
Contributor

Summary

Add default values for BASH_VERSION, ZSH_VERSION, KSH_VERSION.

Shell which sets option nounset occurs following error.

$ /opt/homebrew/opt/asdf/libexec/asdf.sh:68: BASH_VERSION: parameter not set

@kurochan kurochan requested a review from a team as a code owner July 12, 2023 02:12
@hyperupcall
Copy link
Contributor

hyperupcall commented Jul 12, 2023

Nice catch!

To properly address this, the variables cannot be exported and should not be assigned to because this changes the shell environment.

So the :- variable substitutions have to be inline whenever each of those variables are used

@kurochan kurochan force-pushed the add-default-value-shell-version branch from 7c1232c to ebe7310 Compare July 12, 2023 04:21
@kurochan
Copy link
Contributor Author

@hyperupcall
Thanks for the review. It was indeed causes breaking changes 😱
Fixed at ebe7310 .

@hyperupcall
Copy link
Contributor

LGTM! Waiting for other maintainers to give feedback / take action

@hannestyden
Copy link

Happy to see that this fix is available already so I don't have to file it myself. 🙇:blush:

@kurochan
Copy link
Contributor Author

kurochan commented Sep 6, 2023

But unfortunately, it has not yet been approved and released 😢

@hyperupcall
Copy link
Contributor

I would merge this if I could - the maintainers seem to be merging things less frequently these days. For now, I have cherry-picked this commit (and others from other PRs) into fox-incubating/asdf, one of my forks.

All I can do is release a new version from my fork. I plan to add other features as well.

Copy link
Contributor

@jthegedus jthegedus left a comment

Choose a reason for hiding this comment

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

Thanks! And thanks for the review @hyperupcall 🙇‍♂️

@jthegedus jthegedus merged commit 4d5f22d into asdf-vm:master Sep 10, 2023
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants