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

Re-enable env var directives in the .NET CLI #37827

Conversation

baronfel
Copy link
Member

@baronfel baronfel commented Jan 8, 2024

Fixes #37686

Description

This re-adds the EnvironmentVariableDirective, which used to be a default part of the CLI configuration but was removed in a refactoring.

Customer Impact

Customers that set environment variables using this mechanism (which is OS agnostic and scoped only to a single command) suddenly had their commands break. After this change, their scripts will work again.

Regression?

Yes, a System.CommandLine update broke silently in the 8.0.100 preview cycles.

Risk

Low

Tests

Automated tests were added to ensure the env-var setting functionality is verified at the SDK-level, not just the S.CL-level.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Infrastructure untriaged Request triage from a team member labels Jan 8, 2024
@baronfel baronfel changed the title Update dependencies from https://github.com/dotnet/msbuild build 2024… Re-enable env var directives in the .NET CLI Jan 8, 2024
@baronfel
Copy link
Member Author

baronfel commented Jan 8, 2024

We'll need to get servicing approval for this one

@baronfel baronfel added Servicing-consider and removed untriaged Request triage from a team member labels Jan 12, 2024
@baronfel
Copy link
Member Author

Approved for servicing over email, merging!

@baronfel baronfel added this to the 8.0.2 milestone Jan 12, 2024
@baronfel baronfel merged commit fd843c8 into dotnet:release/8.0.1xx Jan 12, 2024
16 checks passed
@MiYanni
Copy link
Member

MiYanni commented Jan 12, 2024

@baronfel I dug a little in the history and it doesn't look like this was "forgotten" per se. What seems to have happened is the structure of how it was indicated in the S.CL changed in a major breaking change in the June timeframe. In the past, it is likely this functionality was included in some other directive, as I don't see it explicitly being enabled in our code from this pull request. But now, they've made it its own explicit directive and that wasn't added because, since we didn't have tests for it, it didn't seem to be needed/required. So, a series of changes/oversights lead to the functionality not being included. 🙃 Thank you for adding the test as that will help the functionality remain, even if S.CL changes more under the hood.

@baronfel baronfel deleted the reintroduce-env-var-directive-support branch January 12, 2024 17:25
@xt0rted
Copy link
Contributor

xt0rted commented Jan 12, 2024

@MiYanni back when I started testing out System.CommandLine I looked through the code numerous times trying to figure out how this was wired up since there were no explicit calls to .UseEnvironmentVariableDirective() or .UseDefaults(). I could never find anything but my theory is it was an accidental side-effect of how commands are sometimes invoked. In this project command.InvokeAsync(...) is called from a number of locations, and depending on how that is called you can get sent down a path that ends up calling .UseDefaults() which includes .UseEnvironmentVariableDirective().

@MiYanni
Copy link
Member

MiYanni commented Jan 12, 2024

@xt0rted Ohhhh! Okay, that's good to know. I didn't know it was .UseDefaults() that was triggering this to happen previously. Any refactoring efforts can have some things fall through the cracks. But adding more tests like what was done here helps those things get caught before they go out the door. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants