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 profile collection on non-Windows, add PS 7 profiles #1260

Merged
merged 5 commits into from
Aug 23, 2019

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Jun 14, 2019

PR Summary

  • Fixes some issues with profile collection in PS 7
  • Makes the OS Name field more descriptive and adds a Description field to capture the current value
  • Adds profiles for PS 7-preview.1 on Windows 10, Windows Server 2019 and Ubuntu 18.04.2

Services PowerShell/PowerShell#9831.

PR Checklist

Copy link
Collaborator

@bergmeister bergmeister left a comment

Choose a reason for hiding this comment

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

Some minor comments, looks OK overall though. Please wait with the merge until development is merged into master for 1.18.1. Even after that we are thinking of changing the default branch to master, so just hold off from merging atm please

@@ -82,7 +82,7 @@ public CompatibilityProfileCollector Build(SMA.PowerShell pwsh)
}
}

private static readonly Version s_currentProfileSchemaVersion = new Version(1, 1);
private static readonly Version s_currentProfileSchemaVersion = new Version(1, 2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am guessing the code works in such a way that and old schema (change only in minor version) is backwards compatible, i.e. old profiles still work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a comment explaining this

{
try
{
using (FileStream fileStream = File.OpenRead(path))
Copy link
Collaborator

Choose a reason for hiding this comment

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

DRY:

Suggested change
using (FileStream fileStream = File.OpenRead(path))
using (var fileStream = File.OpenRead(path))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's no repetition here; FileStream only occurs once on the line and it's not obvious that File.OpenRead returns that type

/// <returns>A dictionary with the keys and values of all the release info files on the machine.</returns>
public static IReadOnlyDictionary<string, string> GetLinuxReleaseInfo()
{
var dict = new Dictionary<string, string>();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a more descriptive name could be useful? What about creating it in a case insensitive way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A more descriptive name for the method? Not sure what to call it other than this, but open to suggestions.

I thought about case-sensitivity, but ultimately this is Linux-specific and there's nothing to stop two keys being added that differ only by case

Copy link
Collaborator

Choose a reason for hiding this comment

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

No, for dict

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh! Got it :)

}
}
}
catch (IOException)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe a comment why this could happen and is OK could be helpful.

@rjmholt
Copy link
Contributor Author

rjmholt commented Jun 14, 2019

Many of the changes I think here are me moving methods around, might be worth turning off whitespace changes for the diff