-
Notifications
You must be signed in to change notification settings - Fork 185
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: Make sure that environments are not completely overridden #2396
fix: Make sure that environments are not completely overridden #2396
Conversation
9639c5a
to
d8b53f0
Compare
d8b53f0
to
9ac30f9
Compare
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.
Thank you! The PR looks good from an implementation stand point!
However this will be a breaking change of the configuration.
Users would need to switch from:
[activation]
scripts = ["default.sh"]
[target.win-64.activation]
scripts = ["default.bat"]
To:
[target.unix.activation]
scripts = ["default.sh"]
[target.win.activation]
scripts = ["default.bat"]
To not have a broken experience.
I'll contact the team what we'll do with this.
I get the implementation from the perspective of environment variables. I would indeed want to use the environment variables from all the targets and have them override each other based on environment variable name. However, for activation scripts, I would not want to combine them. I think we should make this distinction is properly made. Alternatively we could also change the |
I do like the script map idea; it also occurred to me. However; my concern is that existing user configs would be broken by this. |
I've reverted the changes to activation scripts in 720db9e; but kept ones for envs to get this in faster. |
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.
Thank you for the contribution.
Fixes #2383