Skip to content

OS constraint implementation #4691

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

Merged
merged 1 commit into from
May 16, 2022
Merged

Conversation

vlada-shubina
Copy link
Member

@vlada-shubina vlada-shubina commented May 6, 2022

Problem

#3107 - OS constraint

Checks:

  • Added unit tests
  • Added #nullable enable to all the modified files ?

@vlada-shubina vlada-shubina changed the base branch from main to feature/constraints May 6, 2022 15:39
@vlada-shubina
Copy link
Member Author

@baronfel @JanKrivanek looking for the feedback here: do we need to have more features here?

  • support other OS
  • support exact names (Ubuntu, Alpine ...)
  • support OS versions/version range?

As of now only Windows, Linux, OSX are supported (mapped to corresponding OSPlatform).

Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

I admit I have no insight and understanding of Maui use cases and requirements for our constraints feature. But I'm worried whether this implementation can fullfill them

@baronfel
Copy link
Member

Agree with the OperatingSystem-based approach. We don't need to handle OS versions, there's no concrete use case for that that I can see. I'd just make sure that case-insensitive comparisons are handled and then this is good to go IMO!

@vlada-shubina vlada-shubina changed the base branch from feature/constraints to main May 12, 2022 16:09
@vlada-shubina vlada-shubina marked this pull request as ready for review May 12, 2022 16:09
@vlada-shubina vlada-shubina requested a review from a team as a code owner May 12, 2022 16:09
@vlada-shubina vlada-shubina requested a review from JanKrivanek May 12, 2022 16:10
@vlada-shubina
Copy link
Member Author

Opening for final review, now targeting main.

I decided to stay with OSPlatform as it has definition for various OS Platforms, in contrary to OperatingSystem class. Both are supported in .NET Framework and .NET (Core), so no preference here.

@vlada-shubina vlada-shubina changed the title [feature/constraints] OS constraint OS constraint implementation May 12, 2022
Copy link
Member

@JanKrivanek JanKrivanek left a comment

Choose a reason for hiding this comment

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

Looks good!

@vlada-shubina vlada-shubina merged commit fe2b1cd into dotnet:main May 16, 2022
@vlada-shubina vlada-shubina deleted the os-constraint branch May 16, 2022 13:54
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.

3 participants