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

Block bad installs and improve authoring messages #1500

Merged
merged 3 commits into from Apr 9, 2018

Conversation

Projects
None yet
2 participants
@mlorbetske
Copy link
Member

mlorbetske commented Apr 4, 2018

Fixes #1492

Fixes #1498

Example output from the repro in #1498

D:\Projects\repro>dotnet new3 -i .\ThisFails --trace:authoring
Authoring: '/ClientApp/node_modules/caniuse-db/features-json/template.json' was not installed
Authoring:     Missing 'identity' in '/ClientApp/node_modules/caniuse-db/features-json/template.json'
Authoring:     Missing 'name' in '/ClientApp/node_modules/caniuse-db/features-json/template.json'
Authoring:     Missing 'sourceName' in '/ClientApp/node_modules/caniuse-db/features-json/template.json'
Authoring:     Missing 'shortName' in '/ClientApp/node_modules/caniuse-db/features-json/template.json'
Usage: new3 [options]

Options:
  -h, --help          Displays help for this command.
  -l, --list          Lists templates containing the specified name. If no name is specified, lists all templates.
  -n, --name          The name for the output being created. If no name is specified, the name of the current directory is used.
  -o, --output        Location to place the generated output.
  -i, --install       Installs a source or a template pack.
  -u, --uninstall     Uninstalls a source or a template pack.
  --nuget-source      Specifies a NuGet source to use during install.
  --type              Filters templates based on available types. Predefined values are "project", "item" or "other".
  --force             Forces content to be generated even if it would change existing files.
  -lang, --language   Filters templates based on language and specifies the language of the template to create.


Templates                Short Name       Language          Tags
--------------------------------------------------------------------------
Console Application      console          [C#], F#, VB      Common/Console
Class library            classlib         [C#], F#, VB      Common/Library
Unit Test Project        mstest           [C#], F#, VB      Test/MSTest
xUnit Test Project       xunit            [C#], F#, VB      Test/xUnit
BadTemplate              badtemplate      [C#]              Web
global.json file         globaljson                         Config
NuGet Config             nugetconfig                        Config
Web Config               webconfig                          Config
Solution File            sln                                Solution

Examples:
    dotnet new3 mstest
    dotnet new3 classlib --framework netcoreapp2.0
    dotnet new3 --help

D:\Projects\repro>

@mlorbetske mlorbetske requested a review from seancpeters Apr 4, 2018

@mlorbetske

This comment has been minimized.

Copy link
Member

mlorbetske commented Apr 4, 2018

Posting the sequence of events that were intended for the PR and not #1492 /facepalm

(Added WIP tag)

Message 1:
This is causing tests to fail, either the new restrictions will need to be relaxed or the test assets will need to be fixed up. The former approach will likely be taken to avoid impacting existing templates.

Message 2:
Turns out the test to avoid regressing #651 came in handy 😄

I've updated the validations to add warnings for missing common properties that won't fail the install and corrected some messaging. An update will be coming soon.

(Pushed new commit)

(Removed WIP tag)


if ((templateModel.ShortNameList?.Count ?? 0) == 0)
{
errorMessages.Add(string.Format(LocalizableStrings.Authoring_MissingValue, "shortName", templateFile.FullPath));

This comment has been minimized.

@seancpeters

seancpeters Apr 9, 2018

Collaborator

Should this be just a warning? I wasn't clear if we'd finalized whether short name should be required or not.

@seancpeters
Copy link
Collaborator

seancpeters left a comment

I'd recommend moving the validation logic to its own method. TryGetTemplateFromConfigInfo() is getting quite long and multiple responsibility.

mlorbetske added some commits Apr 4, 2018

Relax restrictions so as to not break tests
Add warning class messages

Fix messaging around rejecting the template since it's not necessarily called in an install flow

@mlorbetske mlorbetske force-pushed the mlorbetske:dev/mlorbe/Issues1492and1498 branch from 4f26230 to 4dc36c8 Apr 9, 2018

@mlorbetske mlorbetske merged commit 1442887 into dotnet:stabilize Apr 9, 2018

11 checks passed

CentOS7.1 x64 Debug Build Build finished.
Details
Debian8.2 x64 Debug Build Build finished.
Details
Linux x64 Release Build Build finished.
Details
RHEL7.2 x64 Release Build Build finished.
Details
Ubuntu x64 Release Build Build finished.
Details
Ubuntu16.04 x64 Debug Build Build finished.
Details
Ubuntu16.10 x64 Debug Build Build finished.
Details
WIP ready for review
Details
Windows_NT x64 Release Build Build finished.
Details
Windows_NT x86 Debug Build Build finished.
Details
license/cla All CLA requirements met.
Details

@mlorbetske mlorbetske deleted the mlorbetske:dev/mlorbe/Issues1492and1498 branch Apr 9, 2018

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