Issue#803 #804

ILMTitan · 2017-09-20T20:50:34Z

Fixes #803.
Determine the installed .NET Core Sdk versions and only allows ASP.NET Core templates for those that are supported.

…and add unit tests.

codecov · 2017-09-20T20:54:57Z

Codecov Report

Merging #804 into master will increase coverage by 0.26%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #804      +/-   ##
==========================================
+ Coverage   11.57%   11.84%   +0.26%     
==========================================
  Files         503      504       +1     
  Lines       12289    12316      +27     
==========================================
+ Hits         1423     1459      +36     
+ Misses      10866    10857       -9

Impacted Files	Coverage Δ
...CloudExtension/VsVersion/VS15/ToolsPathProvider.cs	`0% <ø> (ø)`	⬆️
...ateChooserDialog/TemplateChooserWindowContent.xaml	`0% <ø> (ø)`	⬆️
...CloudExtension/VsVersion/VS14/ToolsPathProvider.cs	`0% <ø> (ø)`	⬆️
.../TemplateChooserDialog/TemplateChooserViewModel.cs	`100% <100%> (ø)`	⬆️
...eCloudExtension/VsVersion/ToolsPathProviderBase.cs	`100% <100%> (ø)`
...n/GoogleCloudExtension/VsVersion/VsVersionUtils.cs	`30.76% <100%> (+30.76%)`	⬆️
...rds/Dialogs/TemplateChooserDialog/AspNetVersion.cs	`90.24% <100%> (+6.91%)`	⬆️
...rds/Dialogs/ProjectIdDialog/PickProjectIdWindow.cs	`0% <0%> (ø)`	⬆️
...lateWizards/GoogleProjectTemplateSelectorWizard.cs	`100% <0%> (ø)`	⬆️
...ogs/TemplateChooserDialog/TemplateChooserWindow.cs	`0% <0%> (ø)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 790b6af...bf2fd7e. Read the comment docs.

ivannaranjo

Small change regarding the versioning of the SDK vs. the versioning of the runtimes. They do not match 1 to 1, we should encode that in the code somewhere.

ivannaranjo · 2017-09-21T12:56:18Z

...xtension/GoogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/AspNetVersion.cs

        /// </summary>
        public static readonly AspNetVersion AspNet4 = new AspNetVersion("4", false);

+        private static readonly Version s_sdkVersion10 = new Version(1, 0);


Nit, I recommend separating the major and minor versions in the name, as in s_sdkVersion1_1, I think it will be more readable.

Done.
Changing my resharper settings for private static fields to be s_lowerCamelCase_UnderscoresTolerant.

ivannaranjo · 2017-09-21T12:56:35Z

...xtension/GoogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/AspNetVersion.cs

-            {
-                AspNetCore1Preview
-            };
+            return new List<AspNetVersion> {AspNetCore1Preview};


Nit, you could use the => syntax for this.

ivannaranjo · 2017-09-21T12:58:24Z

...xtension/GoogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/AspNetVersion.cs

-            return new List<AspNetVersion>
+            Version result;
+            List<Version> sdkVersions = VsVersionUtils.ToolsPathProvider.GetNetCoreSdkVersions()
+                .Where(version => System.Version.TryParse(version, out result))


Aren't you parsing the versions twice with this expression? Seems that you are forcing the use of LINQ when a simple foreach loop where you store the versions that can be parsed would do.

ivannaranjo · 2017-09-21T13:00:48Z

...xtension/GoogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/AspNetVersion.cs

        /// </summary>
        public static readonly AspNetVersion AspNet4 = new AspNetVersion("4", false);

+        private static readonly Version s_sdkVersion10 = new Version(1, 0);


Also, the versions of the SDK do not match the versions of the runtime. The current supported SDKs are 1.0.4 and 2.0.0. Where the 1.0.4 SDK supports .NET Core 1.0.5 and 1.1.2. The .NET Core SDK 2.0.0 supports the .NET Core runtime 2.0.0. There's no .NET Core SDK 1.1.x.

Yes it's confusing and silly, but that what it is. You should decouple this concepts here.

It looks like the built in ASP.NET Core template dialog allows ASP.NET Core 1.0 and 1.1 when any 1.x SDK is installed. Changing my code to match.

ivannaranjo · 2017-09-21T13:02:45Z

...ogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/TemplateChooserViewModel.cs

+        {
+            IEnumerable<string> sdkVersions = VsVersionUtils.ToolsPathProvider.GetNetCoreSdkVersions();
+            switch (GoogleCloudExtensionPackage.VsVersion)
+                {


Weird indentation of { and } for the switch statement.

ivannaranjo · 2017-09-21T13:04:14Z

GoogleCloudExtension/GoogleCloudExtension/VsVersion/ToolsPathProviderBase.cs

+            }
+            return Directory.EnumerateDirectories(sdkDirectoryPath)
+                .Select(Path.GetFileName)
+                .Where(sdkVersion => !NugetFallbackFolderName.Equals(sdkVersion));


I think you can just !=, .NET has the right semantics for comparing strings.

ILMTitan · 2017-09-21T18:03:48Z

PTAL.

ivannaranjo · 2017-09-27T09:30:14Z

...xtension/GoogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/AspNetVersion.cs

-            return new List<AspNetVersion>
+            List<Version> sdkVersions = GetParsedSdkVersions().ToList();
+            var aspNetVersions = new List<AspNetVersion>();
+            if (sdkVersions.Any(version => version >= s_sdkVersion1_0))


Woudn't this mean that having SDK 2.0 will support .NET Core 1.0 and 1.1? That is not true, see the download archive, the .NET Core SDK 2.0.0 only supports .NET Core 2.0.

The built in ASP.NET Core templates allow you to create projects for ASP.NET Core 1.0 and 1.1 with only SDK 2.0 installed.

A computer with only the .NET Core SDK version 2.0 installed can compile and run projects targeting .NET Core 1.0 and 1.1.

Actually no, the .NET Core 2.0.0 SDK only supports .NET Core 2.0 runtime, you cannot create apps that target netcoreapp1.1 with it. Also the .NET Core 2.0 runtime cannot run .NET Core 1.1 (or 1.0) apps, the .NET Core runtime is not backwards compatible that way.

You are right about the running part. I tired it on a fresh install, uninstalled SDK 1.1.0, installed SDK 2.0.0. The built in ASP.NET Core 1.0 and 1.1 templates were available and built correctly, but failed when attempting to run.

ivannaranjo · 2017-09-27T09:31:10Z

...xtension/GoogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/AspNetVersion.cs

-                AspNetCore1Preview
-            };
-        }
+            AspNetCore1Preview


You should check for the presence of the preview SDK as well no?

I do in another way.
I am changing the code to be more consistent for VS2015 and VS2017.

ivannaranjo · 2017-09-27T09:33:08Z

GoogleCloudExtension/GoogleCloudExtension/VsVersion/ToolsPathProviderBase.cs

+            }
+            return Directory.EnumerateDirectories(sdkDirectoryPath)
+                .Select(Path.GetFileName)
+                .Where(sdkVersion => NugetFallbackFolderName != sdkVersion);


You could perform the filtering here, removing those directory names that do not denote versions. I think that the callers should not be the ones performing this filtering since this method is supposed to return valid versions.

Are you talking about the Version.TryParse(input, out version) filtering? The problem with that is 1.0.0-preview2 is not a valid Version, even though it is an SDK version.

One option would be to include a third party SemVer package, and filter on being able to parse that.

You could special case that particular SDK version (since it is the only one that VS 2015 really supports).

I was also referering to the fact that this method should only return valid SDK versions installed, as it is written right now it leaks the fact that it is listing directories.

…ersion method to match VS2017. Make .NET Core visable based on availability of ASP.NET Core versions.

googlebot · 2017-09-28T00:01:07Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

ivannaranjo · 2017-09-28T12:58:05Z

Please look again at the .NET Core 2.0 support issues.

…0, 2.0).

googlebot · 2017-09-29T00:36:49Z

CLAs look good, thanks!

ILMTitan · 2017-09-29T00:36:55Z

PTAL.

ivannaranjo · 2017-09-29T10:14:16Z

...xtension/GoogleCloudExtension/TemplateWizards/Dialogs/TemplateChooserDialog/AspNetVersion.cs

        /// </summary>
        public static readonly AspNetVersion AspNet4 = new AspNetVersion("4", false);

+        private static readonly Version s_sdkVersion1_0 = new Version(1, 0);


Another wrench thrown your way, the latest SDK is actually 1.1.4 and covers both .NET Core runtime 1.0 and 1.1. I think that you are going to have to create a table to codify the releases here, with what versions of .NET Core they target, etc...

This comment is not accurate, your logic (looking for .NET Core SDK between 1.0.x to 2.0.x) works perfectly.

ivannaranjo

Actually I should have read it more detail. You actually have encapsulated the logic quite well.

Jim Przybylinski added 3 commits September 20, 2017 11:18

Work in progress.

f80d77b

Fix the available asp.net core versions to match available sdks. Fix …

cddde92

…and add unit tests.

Fix a formatting error.

0f8976f

ILMTitan self-assigned this Sep 20, 2017

ILMTitan requested review from Deren-Liao and ivannaranjo September 20, 2017 20:50

Add a test case to hit a otherwise missing line.

b45481b

ivannaranjo suggested changes Sep 21, 2017

View reviewed changes

Resolve PR comments.

0e9bffb

ivannaranjo suggested changes Sep 27, 2017

View reviewed changes

Move the .NET Core filtering for VS2015 into the GetVs2015AspNetCoreV…

bd01a29

…ersion method to match VS2017. Make .NET Core visable based on availability of ASP.NET Core versions.

Change the restrition on the 1.0 and 1.1 templates to require SDK [1.…

bf2fd7e

…0, 2.0).

ivannaranjo suggested changes Sep 29, 2017

View reviewed changes

ivannaranjo approved these changes Sep 29, 2017

View reviewed changes

ILMTitan merged commit 60e106a into GoogleCloudPlatform:master Sep 29, 2017

ILMTitan deleted the issue#803 branch September 29, 2017 21:46

Issue#803 #804

Issue#803 #804

Uh oh!

Conversation

ILMTitan commented Sep 20, 2017

Uh oh!

codecov bot commented Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ivannaranjo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ILMTitan Sep 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ILMTitan commented Sep 21, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ILMTitan Sep 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

googlebot commented Sep 28, 2017

Uh oh!

ivannaranjo commented Sep 28, 2017

Uh oh!

googlebot commented Sep 29, 2017

Uh oh!

ILMTitan commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ivannaranjo left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

codecov bot commented Sep 20, 2017 •

edited

Loading

ILMTitan Sep 21, 2017 •

edited

Loading

ILMTitan Sep 27, 2017 •

edited

Loading