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.

Sign up for GitHub

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

Jump to bottom

Add a warning message for PMC install/update -source #1976

Merged
merged 9 commits into from
Feb 2, 2018
Merged

Add a warning message for PMC install/update -source #1976

merged 9 commits into from
Feb 2, 2018

Conversation

nkolev92
Copy link
Member

@nkolev92 nkolev92 commented Jan 24, 2018 •
edited
Loading

Bug

Related: NuGet/Home#5763
Regression: No
If Regression then when did it last work:
If Regression then how are we preventing it in future:

Fix

Details:

Add a warning message when someone runs install -source, that it's not respected for PR projects.

Examples for install & update logs.


PM> Update-Package -source https://api.nuget.org/v3/index.json
The 'Source' parameter is not respected for the 'PackageReference' based project(s) ConsoleApp1,ConsoleApp2. The enabled sources in your NuGet configuration will be used. 
No package updates are available from the current package source for project 'ConsoleApp2'.
No package updates are available from the current package source for project 'ConsoleApp1'.


Attempting to gather dependency information for multiple packages with respect to project 'ClassLibrary3', targeting '.NETFramework,Version=v4.6.1'
Gathering dependency information took 2.53 sec
Attempting to resolve dependencies for multiple packages.
Resolving dependency information took 0 ms
Resolving actions install multiple packages
Resolution was successful but resulted in no action
There are no new updates available.
No package updates are available from the current package source for project 'ClassLibrary3'.
Executing nuget actions took 158.63 ms
Executing nuget actions took 0.01 ms
Executing nuget actions took 0 ms
Time Elapsed: 00:00:03.5206106
PM> 


PM> Install-Package nunit -source https://api.nuget.org/v3/index.json
The 'Source' parameter is not respected for the 'PackageReference' based project(s) PackageReference. The enabled sources in your NuGet configuration will be used. 
Restoring packages for C:\Users\nikolev.REDMOND\Source\Repos\ClassLibrary3\ConsoleApp1\ConsoleApp1.csproj...
Installing NuGet package nunit 3.9.0.
Committing restore...
Writing lock file to disk. Path: C:\Users\nikolev.REDMOND\Source\Repos\ClassLibrary3\ConsoleApp1\obj\project.assets.json
Restore completed in 215.65 ms for C:\Users\nikolev.REDMOND\Source\Repos\ClassLibrary3\ConsoleApp1\ConsoleApp1.csproj.
Executing nuget actions took 122.01 ms
Time Elapsed: 00:00:00.3941650
PM>

Testing/Validation

Tests Added: No
Reason for not adding tests:
Validation done: Manual

jainaashish
jainaashish reviewed Jan 25, 2018
View reviewed changes
src/NuGet.Clients/NuGet.PackageManagement.PowerShellCmdlets/Cmdlets/UpdatePackageCommand.cs Outdated

if (Source != null)
{
var projectNames = string.Join(",", Projects.Where(e => e.ProjectStyle == ProjectStyle.PackageReference).Select(p => NuGetProject.GetUniqueNameOrName(p)));
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this ProjectStyle be set for every case? or how about instead of ProjectStuyle, check for NuGetProject type being BuildIntegrated?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's always set.
I personally prefer to use project style because it's easier to read?

What would the error message be then?
Do we mention both package reference and project json?

jainaashish
jainaashish reviewed Jan 25, 2018
View reviewed changes
src/NuGet.Clients/NuGet.PackageManagement.PowerShellCmdlets/Cmdlets/UpdatePackageCommand.cs Outdated
@@ -116,6 +118,7 @@ protected override void ProcessRecordCore()
await _lockService.ExecuteNuGetOperationAsync(() =>
{
SubscribeToProgressEvents();
ValidateArgumentsAreSupported();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we differentiate between update for all projects vs individual selected project? And then according to selected project, takes the call to show this warning?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's already done in an non-intuitive way.
Had me confused initially as well.
https://github.com/NuGet/NuGet.Client/blob/bf99156406e4e702bd3e11ad1ffe79f7571941d1/src/NuGet.Clients/NuGet.PackageManagement.PowerShellCmdlets/Cmdlets/UpdatePackageCommand.cs#L84-L91

jainaashish
jainaashish reviewed Jan 25, 2018
View reviewed changes
src/NuGet.Clients/NuGet.PackageManagement.PowerShellCmdlets/PackageActionBaseCommand.cs Outdated
@@ -265,6 +266,15 @@ protected void NormalizePackageId(NuGetProject project)
Id = metadata.First().Identity.Id;
}

protected virtual void ValidateArgumentsAreSupported()
{
if (Project.ProjectStyle == ProjectStyle.PackageReference && Source != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, using NuGetProject type might makes more sense.

jainaashish
jainaashish reviewed Jan 25, 2018
View reviewed changes
src/NuGet.Clients/NuGet.PackageManagement.PowerShellCmdlets/PackageActionBaseCommand.cs Outdated
{
if (Project.ProjectStyle == ProjectStyle.PackageReference && Source != null)
{
var warning = string.Format(CultureInfo.CurrentUICulture, Resources.Warning_SourceNotRespectedForProjectStyle, nameof(Source), Project.ProjectStyle, NuGetProject.GetUniqueNameOrName(Project));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:break the line?

jainaashish
jainaashish suggested changes Jan 25, 2018
View reviewed changes
Copy link
Contributor

@jainaashish jainaashish left a comment

Choose a reason for hiding this comment

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

Unfortunately ProjectRestoreStyle isn't the reliable way to figure out format type so instead use NuGetProject instance type.

rohit21agrawal
rohit21agrawal reviewed Jan 28, 2018
View reviewed changes
src/NuGet.Clients/NuGet.PackageManagement.PowerShellCmdlets/Cmdlets/UpdatePackageCommand.cs Outdated
@@ -150,6 +153,19 @@ protected override void ProcessRecordCore()
TelemetryService.EmitTelemetryEvent(actionTelemetryEvent);
}

protected override void ValidateArgumentsAreSupported()
Copy link
Contributor

Choose a reason for hiding this comment

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

this isn't as much as validating as it is warning. i suggest change name to something like : WarnForPackageReferenceProject

@nkolev92 nkolev92 force-pushed the dev-addwarningforpmcsourcecommand branch from f39b295 to bb1403d Compare January 31, 2018 23:58
@nkolev92
Copy link
Member Author

nkolev92 commented Feb 1, 2018 •
edited
Loading

🔔
@rohit21agrawal @jainaashish
Updated based on your feedback.

@nkolev92 nkolev92 force-pushed the dev-addwarningforpmcsourcecommand branch from 5d829e8 to fca9172 Compare February 1, 2018 20:59
jainaashish
jainaashish approved these changes Feb 2, 2018
View reviewed changes
Copy link
Contributor

@jainaashish jainaashish left a comment

Choose a reason for hiding this comment

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

👍

@nkolev92 nkolev92 merged commit 9bbd7b1 into dev Feb 2, 2018
@nkolev92 nkolev92 deleted the dev-addwarningforpmcsourcecommand branch February 2, 2018 01:33
@nkolev92 nkolev92 mentioned this pull request Jan 31, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jainaashish jainaashish jainaashish approved these changes

@emgarten emgarten emgarten approved these changes

@rohit21agrawal rohit21agrawal rohit21agrawal approved these changes

@PatoBeltran PatoBeltran Awaiting requested review from PatoBeltran

@zhili1208 zhili1208 Awaiting requested review from zhili1208

@mishra14 mishra14 Awaiting requested review from mishra14

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nkolev92 @rohit21agrawal @emgarten @jainaashish