Warn package authors when uploading a semver2 package through the web site #4239

xavierdecoster · 2017-06-20T08:59:09Z

This PR is a proposal we can start working from. Awaiting final wording from @anangaur.

I'd suggest to have the warning as close as possible to the submit button to ensure maximum visibility (people don't always read messages at the top of the page).

When a package has a semver2 version itself:

When a package has a non-semver2 version, but is considered semver2 due to a dependency declaration:

joelverhagen · 2017-06-20T14:03:51Z

src/NuGetGallery/Views/Packages/VerifyPackage.cshtml

+                <text>
+                    Warning: This package has a SemVer2 package version. <br />
+                    <span style="font-weight: normal; font-style:italic;">
+                        This package will only be available for download with SemVer2-compatible NuGet clients, such as Visual Studio 2017 (version 15.3) and above or NuGet client 4.3.0 and above.<br />


SemVer 2.0.0 instead of SemVer2

Fixed in 57fe6cf

scottbommarito · 2017-06-20T20:57:50Z

src/NuGetGallery/Controllers/PackagesController.cs

            var model = new VerifyPackageRequest
            {
                Id = packageMetadata.Id,
                Version = packageMetadata.Version.ToFullStringSafe(),
                OriginalVersion = packageMetadata.Version.OriginalVersion,
+                HasSemVer2Version = packageMetadata.Version.IsSemVer2,


Can we reuse the code at

NuGetGallery/src/NuGetGallery.Core/SemVerLevelKey.cs

Line 39 in e598224

public static int? ForPackage(NuGetVersion originalVersion, IEnumerable<PackageDependency> dependencies)

?

By doing that, we can reduce the model to just having a IsSemVer2 property, and remove any duplication of this code.

Please note that the message is different when the package has a semver2 version itself, or only has a semver2 package dependency.

I see the value from technical point of view to reuse the SemVerLevelKey class to determine IsSemVer2. However, I also see value in really being able to point the author to exactly what piece of metadata is causing the warning. We can only do that by distinguishing both scenarios. Would like to hear what @anangaur thinks about this one :)

Oh I forgot about the message being different for the two different scenarios. Can we split the SemVerLevelKey.ForPackage method into two helper methods--HasSemVer2Version and HasSemVer2Dependency and then call those methods here?

scottbommarito · 2017-06-20T22:43:06Z

src/NuGetGallery/Views/Packages/VerifyPackage.cshtml

+            </span>
+            <br />
+            <br />
+            <input id="verifyUploadSubmit" type="submit" value="Submit" title="Verify Details &amp; Submit" />


Two things:
1 - I don't like that we have the HTML for this button in two places.
2 - I think it looks a little odd that the button is inside the alert. Additionally, we don't have any buttons inside other alerts.

I think we could resolve both of these issues by moving the button outside of the alert.

I really really want to have this warning as much in the way as possible so authors have to read it..

Moving the button out of the warning banner. Seems in your face enough indeed. As long as it is close to the actionable button of this page so authors can't just skip it "by accident" :)

Yea I didn't want to move the warning away--I like it right next to the button--but I just wanted to move the button out of the yellow. Looks great!

xavierdecoster · 2017-06-21T07:18:18Z

Ping @anangaur

anangaur · 2017-06-21T17:09:27Z

I agree we should make a differentiation. And this differentiation should be conistent across sever and client. The text needs some minor tweaks though and a Fwd link and I will be able to provide the same, later.
I see the following cases:

The package itself is SemVer 2 - Msg1
2a. Package has dependencies declared in SemVer 2 - Msg 2
2b. Package has dependecy declared as SemVer 1 but the package itself is SemVer 2 (due to build metadata) - Msg 2

xavierdecoster · 2017-06-21T18:23:32Z

@anangaur about case 2b: I think what you describe there is what we understood to be a non-goal. There's no way we can detect that a dependency that would be resolved is semver2 for a given non-semver2 dependency version range. That would require analysis of all restore graphs, which is currently not taken into account to determine whether the package is semver2. So we can only detect two cases: 1 and 2a (or just 2 :))

xavierdecoster · 2017-06-21T18:24:23Z

Thanks for the reviews! Holding off until I receive final wording from @anangaur.

anangaur · 2017-06-21T18:33:17Z

@xavierdecoster I thought we can still figure out case 2b not for all dependencies but for 1st level. Atleast thats what I understood based on the discussions with @joelverhagen and @ryuyu

Case 1: We should be able to detect PackageA is SemVer 2 as direct dependency is SemVer 2
PackageA 1.0.0
|--- PackageB 2.0.0 (SemVer 2 as the real version is 2.0.0+buildmetadata)

Case 2: We cannot determine Package A to be SemVer 2 as the dependency is deeper than 1 level. Here PackageC is dependent on a SemVer package and not PackageA directly.

PackageA 1.0.0
|--- PackageC 3.0.0
........| --- PackageB 2.0.0 (SemVer 2 as the real version is 2.0.0+buildmetadata)

xavierdecoster · 2017-06-21T18:58:01Z

@anangaur @joelverhagen @ryuyu let's have the discussion here, as hallway discussions don't make it across the ocean ;)

So, we can indeed query case 1, but isn't that raising a warning on what may be a temporary situation?

The nuspec itself does not contain any semver2 version or version range, and at any point in time, a semver2 package dependency that matches the minimum constraint may be listed or unlisted.

e.g. we may not be raising the warning at upload time when the lowest package version matching the lowerbound is v2.0.1. A week later, 2.0.0+metadata is unlisted, still matching the constraint, turning this package suddenly into a semver2 package (and we didn't show a warning).

The opposite may also be true: we warn about the package being semver2 when 2.0.0+metadata is the lowerbound best match, whilst it may be unlisted some time later and a semver1 package version becomes the best match for this lowerbound (and we displayed a warning that no longer makes sense).

In both cases, the (lack of) warning may be experienced as inconsistent, whilst now we only take into account what is physically in the nuspec. I much prefer consistency and easy to understand warnings :)

shishirx34 · 2017-06-21T19:12:32Z

Correct me here, if I am misunderstanding something

The package itself is SemVer 2 - Msg1
2a. Package has dependencies declared in SemVer 2 - Msg 2
2b. Package has dependecy declared as SemVer 1 but the package itself is SemVer 2 (due to build metadata) - Msg 2

Isn't 1. and 2b. the same described here?

xavierdecoster · 2017-06-21T19:15:37Z

@shishirx34 rephrased (to how I understood it :))

2b. Package has dependecy declared as SemVer 1 but the available dependency matching default dependency resolution Lowest itself is SemVer 2 (due to build metadata) - Msg 2

anangaur · 2017-06-21T19:51:26Z

@xavierdecoster thanks for rephrasing. The flip side of not reporting warning for 2b is that the publisher will be happy but the package will still end up being not compatible on the older clients. Now finding out the real issue here would be non-trivial for the publisher when this issue is reported.

… site

…button out of the warning banner for semver2

xavierdecoster · 2017-06-27T07:41:26Z

See my comment here on Case 3: #4215 (comment)

I do want to point out that none of the server APIs are considering the package to be SemVer 2.0.0 in Case 3.

These packages are not considered to be SemVer 2.0.0 and e.g. NOT HIDDEN on V2 OData endpoints, as all we check is the package's nuspec manifest.

This is one of the edge cases described in the non-goals (paragraph 2) that may lead to (gracefully handled) restore failures on non-SemVer2 compatible clients.

xavierdecoster added Area: Gallery UI Pillar: Experience labels Jun 20, 2017

xavierdecoster added this to the S120 - 2017.6.19 milestone Jun 20, 2017

xavierdecoster self-assigned this Jun 20, 2017

xavierdecoster requested review from joelverhagen, anangaur and skofman1 June 20, 2017 08:59

dnfclas added the cla-already-signed label Jun 20, 2017

joelverhagen reviewed Jun 20, 2017

View reviewed changes

joelverhagen approved these changes Jun 20, 2017

View reviewed changes

scottbommarito suggested changes Jun 20, 2017

View reviewed changes

scottbommarito approved these changes Jun 21, 2017

View reviewed changes

agr mentioned this pull request Jun 26, 2017

Deployment 2017.05.07 #4257

Closed

19 tasks

xavierdecoster added 5 commits June 27, 2017 08:58

Warn package authors when uploading a semver2 package through the web…

683fd05

… site

CR feedback

65ba6e4

fix wording

04f3812

Avoid HTML duplication of submit button on VerifyPackage view + move …

4885a0b

…button out of the warning banner for semver2

Added fwlink + updated wording

6307916

xavierdecoster force-pushed the dev-issue4215 branch from 3fd2eef to 6307916 Compare June 27, 2017 07:28

xavierdecoster merged commit 6cb5fed into dev Jun 28, 2017

xavierdecoster deleted the dev-issue4215 branch June 28, 2017 16:38

skofman1 mentioned this pull request Jul 7, 2017

Deployment 2017.07.13 #4357

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Warn package authors when uploading a semver2 package through the web site #4239

Warn package authors when uploading a semver2 package through the web site #4239

xavierdecoster commented Jun 20, 2017 •

edited

Loading

joelverhagen Jun 20, 2017

xavierdecoster Jun 20, 2017

scottbommarito Jun 20, 2017

xavierdecoster Jun 21, 2017

xavierdecoster Jun 21, 2017

scottbommarito Jun 21, 2017 •

edited

Loading

scottbommarito Jun 20, 2017

xavierdecoster Jun 21, 2017

xavierdecoster Jun 21, 2017

scottbommarito Jun 21, 2017

xavierdecoster commented Jun 21, 2017

anangaur commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017

anangaur commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017 •

edited

Loading

shishirx34 commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017

anangaur commented Jun 21, 2017

xavierdecoster commented Jun 27, 2017

Warn package authors when uploading a semver2 package through the web site #4239

Warn package authors when uploading a semver2 package through the web site #4239

Conversation

xavierdecoster commented Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scottbommarito Jun 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xavierdecoster commented Jun 21, 2017

anangaur commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017

anangaur commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017 • edited Loading

shishirx34 commented Jun 21, 2017

xavierdecoster commented Jun 21, 2017

anangaur commented Jun 21, 2017

xavierdecoster commented Jun 27, 2017

xavierdecoster commented Jun 20, 2017 •

edited

Loading

scottbommarito Jun 21, 2017 •

edited

Loading

xavierdecoster commented Jun 21, 2017 •

edited

Loading