umbraco / Umbraco.Marketplace.Issues

Public issue tracker for Umbraco Marketplace
2 stars 0 forks source link

Idea: Provide a validation tool for Package Developers to test their NuGet and JSON, etc. #27

Closed hfloyd closed 1 year ago

hfloyd commented 1 year ago

According to the documentation, NuGet as a whole is checked every 24 hours - which means that if there is something missing on a NuGet package release, PDs (Package Devs) won't know until the next day, and if they make a correction, it will take another day to know whether that fixed the issue. Additionally, "known" packages won't have their info updated for up to 2 hours - which adds in further delays when trying to fix tagging/formatting, etc.

I think these timelines are just fine for actual import checking - since on any given day, very few packages would be actively changing, but when a PD is actively working on a new release, it would be nice for there to be a way to get information a bit quicker.

I would love for there to be a tool (perhaps right on the documentation page), which would allow you to put a package ID into a text box, and get back a little report of what is found on the package as it is live right now.

Example:

image

hfloyd commented 1 year ago

For instance, I see that my NuGet packages have been imported, but even after I made an update to my website yesterday, the marketplace listing is still not pulling in JSON meta-data from https://dragonflylibraries.com/umbraco-marketplace-dragonfly.umbraco9.schemaimporter.json

I'm not sure if the tool is not finding the page, or if there is a problem reading it...

abjerner commented 1 year ago

Hi @hfloyd

I thought about something like this as well. I like your example 👍 That would be helpful for debugging issues instead of having to wait for the Marketplace.

When the Marketplace was still in beta, I played around with creating some C# models fro generating the necessary JSON for the Marketplace - and then validating the generated JSON against the schema. You can see that here:

https://gist.github.com/abjerner/19767bb967819be301182d7bddbce3ab#file-marketplace-cshtml

I tried validating your JSON file, but didn't even get to that part, as your JSON file seems to HTML encoded. My browser extension shows the JSON fine, but upon further inspection, you can see that it has " instead of ".

If you're using a Razor view for outputting the JSON, it might be that you need to use @Html.Raw(json) instead of just @json. Just a quick guess - could be something else.

image

hfloyd commented 1 year ago

@abjerner - You are my best programming friend ❤ I missed that completely! I use the json-viewer browser extension, which as you pointed out, can obscure what the raw JSON actually contains. 🤦‍♂️

I've pushed an update to the Razor view, and hopefully that will resolve the issue.

I still stand by my Testing tool suggestion, though - It would be so awesome to be able to validate right now that I fixed the problem 🙂

I created C# models for the Marketplace data as well (using QuickType.io for the initial generation), but I'll have to look closer at your Validation code. Could definitely be worth adding!


UPDATE: After integrating schema validation, I see that there was a validation error! One I don't agree with... but - it helped me work around the problem. Thanks again, @abjerner !

nathanwoulfe commented 1 year ago

Hi both. This sounds like a sensible request - @KevinJump raised a similar issue earlier in the project.

Internally, we have a JSON validator/parser written specifically for the marketplace JSON. However, since it is an internal part of the marketplace infrastructure, it doesn't do much (anything really) for returning feedback suitable for use in a UI. Doesn't at the moment, but could likely be extended to do so.

In fact, this sounds more interesting than what I had planned for today, will see what I can make work...

abjerner commented 1 year ago

Awesome 👍

Just out of curiosity, is there a reason for the Marketplace not being open source?

nathanwoulfe commented 1 year ago

@abjerner not sure re OSS, @AndyButland might be able to clarify.

I made some reasonable progress on this last week, but then realised I hadn't considered the scenario where a developer wants to validate an unpublished package, so need to work on that too (would need to validation JSON only, from an upload/input)

AndyButland commented 1 year ago

Sorry, I missed the OSS question @abjerner. We have considered this, but for now have kept things closed source. There were a few reasons for the initial choice to do that. Partly just down to practicalities - it may not have been actually too useful, nor a great contributor experience, in the early days of development, when there's a lot of flux. And partly it's down to their being at least some Umbraco "IP" in here (we've utilised some patterns, code and practices from the Cloud team's services, there's stuff around build pipelines, IaC etc.).

So for now we're taking a middle ground. The code and ability to contribute has been opened up to a small group of contributors - namely the the package team - so there's room via that route for community contribution. And of course we're very open to issues being raised and general feedback. But - at least for now - we haven't gone so far as to open source the whole repository. I'm happy to discuss privately if you'd like to hear more or had any specific contribution that you had in mind - please reach out if so.

abjerner commented 1 year ago

@AndyButland no worries. When there is something the CMS I'm unsure how works, I like to browse through the source code. So my question was mostly at that level - being able to look how things work 😉

hfloyd commented 1 year ago

@nathanwoulfe

I made some reasonable progress on this last week, but then realised I hadn't considered the scenario where a developer wants to validate an unpublished package, so need to work on that too (would need to validation JSON only, from an upload/input)

Haha! Scope-creep! I'm sure a "minimum viable product" version requiring a live NuGet package would be fine 😉

nathanwoulfe commented 1 year ago

@hfloyd a little scope creep never hurt anyone, right? Right?

Anyway, made good progress on this today: image

nathanwoulfe commented 1 year ago

@hfloyd @abjerner and @kevinjump (since you commented in the private repo around same) - validation tools are now on dev if you feel like breaking it 😄 Open to suggestions for any functionality I've not considered.

https://dev.marketplace.umbraco.com/validate

abjerner commented 1 year ago

@nathanwoulfe awesome. But seems the validation request goes to localhost: https://localhost:50372/api/v1.0/validatePackage/Skybrud.Umbraco.Redirects?version=4.0.6&connectionId=undefined

"it works on my machine" 😆

nathanwoulfe commented 1 year ago

Ah yeah that won't work... Update building now, should be betterer.

hfloyd commented 1 year ago

Hmm... I filled in the only "required" field, but the buttons are disabled? image

When I added a version number, it worked great! I guess it can't just get the "latest" version from NuGet, if blank?

From a UI perspective, it might make sense to separate it into two forms - since the big JSON box is really only needed if you don't yet have it on NuGet and want to check the JSON schema via Copy & paste...

Like:

"If your package has been pushed to NuGet, you can check it here:"

NUGET FORM

"If you just want to check your JSON, paste it here for validation:"

JSON FORM

abjerner commented 1 year ago

Looks great 👍

I agree with Heather that a fallback for the most recent version would be ideal. A also noticed a few other minor issues:

Pre-releases Pre-releases are not included in the Marketplace, but I only need that from talking with Andy here on GitHub. The listing page doesn't seem to mention this.

So adding a warning or error for this might make sense.

image

README From what I've understood, the README file is only if you wish to differ from the NuGet README / description (which I like and already using for some of our packages 👍), so the alert should probably be a warning instead of an error. Or maybe show an error if neither the NuGet package has a README or a custom README file.

It might also be worth mentioning that the Marketplace tries both of these URLs - not just the latter: https://packages.skybrud.dk/skybrud.umbraco.redirects/umbraco-marketplace-readme-skybrud.umbraco.redirects.md https://packages.skybrud.dk/skybrud.umbraco.redirects/umbraco-marketplace-readme.md

The warning is also bit wrong, since the package does have a README via the NuGet README file - just not a custom README file on our package website.

image

Updating package details for the Marketplace More a question than an issue: will running the validator for my package update the information that is used on the Marketplace? Or do I have to wait for the normal scheduled task?

Either is fine for me, really, but a way to trigger an update - eg. like Facebook's sharing debugger and similar tools - would be nice to have.

nathanwoulfe commented 1 year ago

Thanks team, very much valid feedback. I've added changes (deployed to dev.marketplace.umbraco.com):

Re updating as part of the validator task, I think we'd need to keep it separate. If I validate my new package 10 times in a feeble attempt to get the JSON correct, we don't want the import task running 10 times for that package. We could look at allowing triggering an update if the package passes validation, but that still opens the service to potential abuse.

More likely we'd look at delivering this as part of the next stage, which will be adding additional tools for developers to manage their own packages.

greystate commented 1 year ago

This is stellar work 🙌

One ting that seriously confuses me is where/how the Marketplace determines the v10 & v11 etc. compatibility... Would be nice if this tool could somehow output something to the effect of "This package seems to be compatible with Umbraco versions 10 & 11" or similar.

abjerner commented 1 year ago

Hi @greystate

Regarding the compatibility, it's based on the Umbraco dependencies of your NuGet package.

For instance with Vokseverk.ColorSelector, the dependency is for Umbraco 10 and anything above.

image

For most of our Limbo/Skybrud packages, I've started adding an upper bound - eg. as shown here:

image

You can see my .csproj file here:

https://github.com/skybrud/Skybrud.Umbraco.Redirects/blob/v4/main/src/Skybrud.Umbraco.Redirects/Skybrud.Umbraco.Redirects.csproj#L36-L38

The Marketplace also looks at previous releases, but I'm not entirely sure how that works. But at least it detects that Skybrud Redirects supports U8, U9, U10 and 11 (across three different major versions).

greystate commented 1 year ago

Yeah - that's the confusing part for me - both of my packages are referencing Umbraco >= 10.0.0 on Nuget, but one of them is v10 only and the other is v10 + v11:

Two package cards from the Marketplace showing the ColorSelector and KeyValue Editor packages

While trying to figure this out, I uploaded a couple of versions, and from what I remember, it seemed as if the data being updated was out of sync. As if one of them (Nuget vs. marketplace JSON file) was one version behind, if that makes sense?

That would certainly explain why the version on Key/Value Editor is currently v10 only, as the previous version on Nuget was specifically set to v10 (because I hadn't been able to test on v11 yet).

nathanwoulfe commented 1 year ago

I'm closing this one as the initial validation work is now released - there's a separate investigation of the package versioning, I'm sure Andy will update as he progresses.

Anything else re validation can be a new issue/discussion 😄

AndyButland commented 1 year ago

@greystate - just to note that I've had a look at this. The problem is coming from an unlisted version of your package, that indicates a version that excludes Umbraco 11.

My first thought was simply that we should ignore unlisted versions. Unfortunately that doesn't look to be as easy as I thought, as it doesn't seem that the NuGet API supports this. I'm using this approach - but this includes unlisted versions in the response with no property or similar to indicate if the version is listed or not.

For this case though we probably need to look at our logic anyway. I'm thinking even if a previous version indicates a lower max supported version, listed or not, if a later one extends the range, we should respect that. Will be looking to update to do that.

AndyButland commented 1 year ago

@greystate - should be sorted now.

greystate commented 1 year ago

Oh that's great! Thanks @AndyButland 👍