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: Make sure that environments are not completely overridden #2396

Merged
merged 3 commits into from
Nov 15, 2024

Conversation

hameerabbasi
Copy link
Contributor

@hameerabbasi hameerabbasi commented Oct 31, 2024

Fixes #2383

@hameerabbasi hameerabbasi changed the title Make sure that environments are not completely overridden. fix: Make sure that environments are not completely overridden Oct 31, 2024
@hameerabbasi hameerabbasi force-pushed the default-platform-mixed-env branch 2 times, most recently from 9639c5a to d8b53f0 Compare October 31, 2024 10:04
Copy link
Contributor

@ruben-arts ruben-arts left a 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.

src/project/environment.rs Outdated Show resolved Hide resolved
@baszalmstra
Copy link
Contributor

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 scripts key to take a map instead of an array so you can have "named" scripts which can be individually overriden by different targets.

@hameerabbasi
Copy link
Contributor Author

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 scripts key to take a map instead of an array so you can have "named" scripts which can be individually overriden by different targets.

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.

@hameerabbasi
Copy link
Contributor Author

hameerabbasi commented Nov 15, 2024

I've reverted the changes to activation scripts in 720db9e; but kept ones for envs to get this in faster.

Copy link
Contributor

@ruben-arts ruben-arts left a 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.

@ruben-arts ruben-arts merged commit ff5ae24 into prefix-dev:main Nov 15, 2024
40 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.

Global activation env vars ignored when platform-specific ones are provided
3 participants