Revert "osProvider.Os to include Mac (#1843)"

[bwateratmsft]

This reverts commit 9a74120. This change is way too high risk this close to locking down for 1.1 release.

[ravipal]

I don't this is as risky as you say. We still have lot of time to validate.

[karolz-ms]

But didn't @ravipal review all places where LocalOsProvier.os is used? There were only a few and the logic is relatively straightforward.

If we still think the original change is too risky, my suggestion would be to create the release branch off of a previous commit, instead of reverting the change.

[bwateratmsft]

But didn't @ravipal review all places where LocalOsProvier.os is used? There were only a few and the logic is relatively straightforward.

If we still think the original change is too risky, my suggestion would be to create the release branch off of a previous commit, instead of reverting the change.

Checking LocalOSProvider.os is not enough because a value was added to PlatformOS. Formerly, 'Linux' really meant Unix, now it means exclusively Linux. Any boolean logic that used that meaning could be affected. There are at least 67 references to PlatformOS, 73 to 'Windows', and 38 to 'Linux', and it affects every feature in the entire product.

There are still changes that need to go into master (like #1841, and anything CTI finds). Why force me to go through the extra work of branching and double-checkins just to keep a change that introduced needless risk without actually fixing any bugs?

[karolz-ms]

The original meaning of PlatformOS.Linux was, in practice, "not Windows". It implies that MacOS and Linux can be treated as the same thing in most cases. But we all know this is not true, and that is why we needed the osProvider.isMac hack. So my expectation for this change is that, by making the semantics much clearer, it will prevent future bugs.

That said, the timing might not be the best. I would still opt in for branching early and cherry-picking instead of reverting, but it is your call.

[bwateratmsft]

Let me be clear--I don't disagree with the change itself in #1843, I think it's perfectly fine to do and should be done--just not right now. CTI does a ton of ad-hoc testing from master branch, and it's something that's sort of cumulative over a release or several releases. (Aside: I suspect most of the bugs they find are via this ad-hoc testing, so IMO it has high value.) During the validation pass, they are focused primarily on explicit test cases and not so much ad-hoc testing--so a change with implications this big "resets" a lot of that ad-hoc testing. It's a great candidate for going in early in a release (same as we do with changes to package-lock.json), so it can benefit from all of the ad-hoc testing over the span of that release.

[ravipal]

As I mentioned before this change is not risky and we will find the issues in the CTI testing if any got introduced due to this change. Earlier we were using hack (OS and isMac) to identify the right OS and this will make it clear.

[bwateratmsft]

Adding a value to what was a two-value enum used throughout the product is pretty much the definition of risky. And, as I said, CTI does lots of ad-hoc testing during the course of a release, and this would be resetting all of that.

[ravipal]

Look at the changes made. It all looks straight forward and made it clear.

[bwateratmsft]

Did you inspect every single case where we use PlatformOS, any variables of that type, or any of its values ('Linux', 'Windows')?