[GitHubRelease] Get releases using semver (similar logic to tags) #3144

RedSparr0w · 2019-03-03T22:47:45Z

This PR updates the GitHub releases badge to use our latestVersion function instead of using the most recent release by date (similar to the tag badge).

You can still get the latest release using /release-latest/:user/:repo
(although this excludes pre-releases)

shields-ci · 2019-03-03T22:49:14Z

	Messages
📖	✨ Thanks for your contribution to Shields, @RedSparr0w!

Generated by 🚫 dangerJS against 0f9bbb5

paulmelnikow · 2019-03-05T04:16:15Z

For clarification, is release-latest showing the most recent by date, while release is showing the latest according to semver?

RedSparr0w · 2019-03-05T04:26:39Z

For clarification, is release-latest showing the most recent by date, while release is showing the latest according to semver?

/release-latest/ basically does the same as our current /release/ badge,
which will show whichever release is returned by GitHubs /releases/latest endpoint
(I'm not entirely certain how this is determined, looks to be just date)
for example:
web: https://github.com/guzzle/guzzle/releases/latest → v5.3.3
api: https://api.github.com/repos/guzzle/guzzle/releases/latest → v5.3.3

And the updated /release/ badge will be based off semver (fall back to charcode value)
web: https://github.com/guzzle/guzzle/releases → v6.3.3
api: https://api.github.com/repos/guzzle/guzzle/releases → v6.3.3

paulmelnikow · 2019-03-05T04:32:27Z

/release-latest/ basically does the same as our current /release/ badge,

GitHub API aside, I usually think of "latest" as meaning semver-latest.

Instead of using /release-latest/ for the old behavior, how about /release-most-recent/ or /release-newest/?

chris48s · 2019-03-05T20:48:51Z

I think /releases is actually correct as it stands. By default, our releases badge "agrees" with what GitHub says is the latest release, and I think that is correct behaviour. Assuming semantic versioning is a great idea if we're talking about NPM packages or rust crates for example (where SemVer is enforced by the platform), but GitHub releases and tags are just a string. I can tag my release 2_7_3_2 or 2.2b1 or 2018.11.26 or android-273 and that's totally fine.

It would definitely be useful to add a /releases endpoint that orders by semver, but I don't think we should break for existing users, particularly with a very widely used badge like this.

An additional complication is that our tag and release badges are inconsistent. Right now we've got:

Releases

/releases/ - order by date
/releases-pre/ - order by date, include pre-releases (based on the "This is a pre-release" tickbox)

Tag

/tag/ - assumes the tag name is a Semantic Version
/tag-pre/ - assumes the tag name is a Semantic Version, include pre-releases (using SemVer standards for what is a "pre-release")
/tag-date/ - order by date

Its unfortunate that the base /releases and /tag use different defaults. If we were starting from scratch today, we probably wouldn't make those decisions :(

Finally, if we do go down the route of breaking BC on /releases and switch it to assume semver, lets put the new "order by date" endpoint on /releases-date/ so we at least gain consistency with /tag in the URL schema by doing it.

chris48s · 2019-03-05T21:18:39Z

a very widely used badge like this

Just to quantify this with some stats from the excellent new services dashboard 📊 , over the last week github_release was the 5th most popular badge with 959.33 K requests so its a change that would have quite a wide impact.

RedSparr0w · 2019-03-05T21:37:48Z

I was thinking of using /release-date but unfortunately that's already a badge 😆

Do you think it may be best if I restore the previous /release functionality,
Then create /release-semver & release-semver-pre? endpoints?
This way you are explicitly stating you want the latest version by semver.

RedSparr0w · 2019-03-05T22:22:43Z

Also we could update the tag endpoint to use the GraphQL API v4:
example tag query (sorted by date)
But i'm not quite sure how to use that correctly yet.

paulmelnikow · 2019-03-06T01:40:27Z

Another option is to use a query param, like ?semver. I'm game to give this a bit more thought…

chris48s · 2019-03-06T19:18:05Z

I was thinking of using /release-date but unfortunately that's already a badge

Well spotted :D

Another option is to use a query param, like ?semver

Yeah. Last time we played around with this, we did discuss ?sort=date / ?sort=semver

#2117
#2157

/release-semver-pre isn't an amazing URL, but I think something like that is preferable to changing the behaviour of /releases, which is going to break something for someone :/

Another suggestion is we could define our consistent URL schema for tag and release badges (accounting for order by date, order by semver and include pre-release cases) as if we were starting with no legacy compatibility to deal with, start again (say at /t and /r, or using some prefix for example?) and make the current endpoints we want to support redirects to those?

paulmelnikow · 2019-03-07T00:43:17Z

Here's a proposed guideline for determining whether to use a query param or a URL path component.

A URL path component should be used for one of these things:

The noun is different
1. "release", "version"
2. "rating", "stars" [though I'm on the fence about whether this should be considered formatting]
An adjective is different
1. "dev" or "prod" dependency
2. "daily" or "monthly"
3. which branch to use, or which issue label to use
There's a choice of which attribute to display (as in the pull request detail badges)

Query params should be used for these three things:

The parameter is related to formatting. [Most of our formatting is handled in query params, so it'll make things more consistent to put all of it there, and is not a big stretch.]
The parameter is for an uncommon attribute, like an alternate registry URL.
The parameter triggers application of alternative logic, like version semantics [or whether prereleases should be included?].
The parameter commonly contains characters that require URL encoding. Query-param encoding is well standardized.

If there aren't objections to the guideline I could open a new PR to pull that into the docs.

paulmelnikow · 2019-03-07T01:20:46Z

changing the behaviour of /releases, which is going to break something for someone :/

Agreed we shouldn't try hard to keep the existing logic working the same for all the users who are using it. 💯

I think /releases is actually correct as it stands. By default, our releases badge "agrees" with what GitHub says is the latest release, and I think that is correct behaviour.

I think that's a reasonable argument, though I'm not completely convinced. Both releases and tags on GitHub are often created out of order. I've often pushed tags to projects retroactively, and I've seen projects switch from Releases to just tags, and vice versa. Since GitHub is not an artifact repository, there is not an expectation that releases or tags are created at the time code is published. Nor can any expectations be made about a specific versioning-scheme.

Another suggestion is we could define our consistent URL schema for tag and release badges (accounting for order by date, order by semver and include pre-release cases) as if we were starting with no legacy compatibility to deal with, start again (say at /t and /r, or using some prefix for example?) and make the current endpoints we want to support redirects to those?

If this helps here, I'd definitely support this.

Shields is an "everything and the kitchen sink" sort of project and it's generally hard + rare that we remove features once they exist. So I think the best thing we can do to keep the mental overhead down and keeping Shields feeling easy to use is to aggressively simplify the interfaces, keep them consistent across services, and provide good defaults that generally shield users from complexity.

Another thing that compounds confusion here is that GitHub uses "releases" in the UI to refer to tags that don't have corresponding releases. This is one of our most frequently asked questions.

One possibility could be to deprecate /tag entirely, and migrate everything to /release:

/release/:user/:repo == /release/:user/:repo?sort=date
/release/:user/:repo?include_prereleases
/release/:user/:repo?sort=semver
/release/:user/:repo?include_tags
/release/:user/:repo?include_tags&sort=semver
/release/:user/:repo?include_tags&sort=semver&include_prereleases

When include_tags is set, it would always use the tags API, not the releases API.

Another option would be to put this all under /v:

/v/:user/:repo == /release/:user/:repo?sort=date
/v/:user/:repo?include_prereleases
/v/:user/:repo?sort=semver
/v/:user/:repo?include_tags
/v/:user/:repo?include_tags&sort=semver
/v/:user/:repo?include_tags&sort=semver&include_prereleases

A couple other points to consider related to the query params:

How sorting might be implemented across other services. Would using something like ?sort=semantic make the parameter more flexible on services that define their own semantic version schemes?
Should we plan to add to the UI multiple-choice support for query params?

chris48s · 2019-03-07T19:40:57Z

The parameter triggers application of alternative logic, like version semantics [or whether prereleases should be included?].

This is mostly the opposite of what we're actually doing though:

bower/v / bower/vpre
chocolatey/v / chocolatey/vpre
clojars/v / clojars/vpre
nuget/v / nuget/vpre
packagist/v / packagist/vpre
powershellgallery/v / powershellgallery/vpre
pub/v / pub/vpre

It would be really good to have some docs around URL schema conventions to refer to when adding new services, also covering common naming conventions (e.g: /v for version, /l for licence, /dt, /dm, /dw, /dd for downloads etc). Broadly though we should probably try to formalise/encode conventions which have already evolved and the bring the exceptions into line, rather than document a set of conventions which most of our badges don't conform to and chuck 301 redirects on everything to match them.

One possibility could be to deprecate /tag entirely, and migrate everything to /release
Another option would be to put this all under /v

Both good suggestions. /v is quite nice - I'd completely forgotten that /v is free in this namspace.

/v/:user/:repo?include_tags&sort=date&include_prereleases is a bit of an odd one, but I guess it just does the same thing as /v/:user/:repo?include_tags&sort=date.

How sorting might be implemented across other services.

I think GitHub is a bit special in this respect. GitLab and BitBucket would fall into the same category, but most of the platforms we support don't because mostly the conventions of the platform or language community defines how you should sort versions:

NPM - order by SemVer.
PyPI - use PEP 440 ordering.
Packagist - use composer versioning + so on.

GitHub is a broad church though and hosts projects which use all of the conventions I've just mentioned and more. That's also why date is the only really safe default here IMO.

paulmelnikow · 2019-03-07T20:38:16Z

This is mostly the opposite of what we're actually doing though:

bower/v / bower/vpre
etc.

Right. If we went in this direction it'd be a good handful of vpre badges that we'd eventually want to migrate. /bower/v/:packageName?include_prereleases is more self-explanatory than /bower/vpre/:packageName so I think there's value in that.

It would be really good to have some docs around URL schema conventions to refer to when adding new services, also covering common naming conventions (e.g: /v for version, /l for licence, /dt, /dm, /dw, /dd for downloads etc). Broadly though we should probably try to formalise/encode conventions which have already evolved and the bring the exceptions into line, rather than document a set of conventions which most of our badges don't conform to and chuck 301 redirects on everything to match them.

Broadly agreed.

The change I'm proposing (i.e. vpre -> ?include_prereleases) is dealing with a set of routes that I don't really like, in a way that's flexible enough to apply to this exception case. It has less to do with codified conventions than with modifying our (uncodified) convention. I'm fine with deferring that to a new issue (i.e. using /github/vpre for now).

/v/:user/:repo?include_tags&sort=date&include_prereleases is a bit of an odd one, but I guess it just does the same thing as /v/:user/:repo?include_tags&sort=date.

One option would be for /v/:user/:repo?include_tags&sort=date to show the latest tag, excluding anything recognizable as a prerelease. I expect we could exclude valid semver, pep440, and composer prereleases without falling into trouble. If we did that, folks who don't want any filtering could use &include_prereleases.

chris48s · 2019-03-10T16:14:39Z

I'm fine with deferring that to a new issue (i.e. using /github/vpre for now).

No I think it is sensible to make the GH tags/releases integration match the convention we want everything to use moving forwards and then bring the rest into line instead of putting these on temporary routes we plan to redirect from in the near future. It would be annoying if a very common badge needs a chain of 2 redirects to serve it (both for maintenance and performance).

Also lets not boil the ocean all at once (we don't need to widen the scope of this PR to "change our entire URL schema") but its worth going through our existing routes and thinking about which ones we would want to change to bring into line with this convention before we adopt it.

I'm going to suggest the following assumption here:

The parameter is for an uncommon attribute, like an alternate registry URL

This seems reasonable, as long as we classify branch and tag as 'common' as opposed to 'uncommon' attributes. i.e: we keep :user/:repo/:branch and don't move to :user/:repo?branch=:branch (even though for most badges :branch is quite rarely used).

On that assumption, other than v/vpre, I think there are relatively few other endpoints we would want to change (lets save the exact endpoints for another issue to avoid getting too bogged down) so I think it does make sense to document and make the change you've suggested 👍

One option would be for /v/:user/:repo?include_tags&sort=date to show the latest tag, excluding anything recognizable as a prerelease

I think I'd be inclined to try and avoid trying to be "clever" if sorting tags by date and just accept a tag is a tag, particularly if sort=date is the default.

paulmelnikow · 2019-03-10T16:53:15Z

One option would be for /v/:user/:repo?include_tags&sort=date to show the latest tag, excluding anything recognizable as a prerelease

I think I'd be inclined to try and avoid trying to be "clever" if sorting tags by date and just accept a tag is a tag, particularly if sort=date is the default.

Don't we already have this feature in github/tag-pre? Should we see if anyone is using it?

All else sounds good.

This seems reasonable, as long as we classify branch and tag as 'common' as opposed to 'uncommon' attributes. i.e: we keep :user/:repo/:branch and don't move to :user/:repo?branch=:branch (even though for most badges :branch is quite rarely used).

Huh. Moving branch to the query would simplify some of our routes – there's one pattern (github dependency version?) it would make less ambiguous. But there would be such a huge number of routes that would need changing. Our of curiosity, is that your thinking as well?

chris48s · 2019-03-10T18:31:03Z

Don't we already have this feature in github/tag-pre ?

No. /tag-pre assumes tags are valid semantic versions (unlike /releases-pre which doesn't - confusing, isn't it 😄 ) so include_tags&sort=semver&include_prereleases would do what /tag-pre currently does.

But there would be such a huge number of routes that would need changing. Our of curiosity, is that your thinking as well?

I think we'd be looking at adding something like ~70 redirects if tag and branch become URL params. IMO that creates more problems than it solves.

paulmelnikow · 2019-03-10T18:40:33Z

I think we'd be looking at adding something like ~70 redirects if tag and branch become URL params. IMO that creates more problems than it solves.

Yea, agreed. 👍

It's worth remembering that we could move part of the route into a query param if we otherwise can't build the route unambiguously. I was just looking at GitHub downloads, which might be a candidate for that: either the tag or the path could move into the query param.

No. /tag-pre assumes tags are valid semantic versions (unlike /releases-pre which doesn't - confusing, isn't it 😄 ) so include_tags&sort=semver&include_prereleases would do what /tag-pre currently does.

Ahh I gotcha.

So include_tags&sort=semver&include_prereleases would include prereleases and include_tags&sort=semver would skip them, but when include_tags&sort=date is specified, include_prereleases is ignored because it doesn't really make sense. (Or better yet: it's rejected.)

paulmelnikow · 2019-04-17T19:56:21Z

Sounds like the decision is to tackle this after rewriting this badge for #2863.

Ref #2863 (comment)

use similar logic to tag

0f9bbb5

paulmelnikow changed the title ~~[GitHub] Get releases using semver (similar logic to tags)~~ [GitHubRelease] Get releases using semver (similar logic to tags) Mar 5, 2019

paulmelnikow added the service-badge New or updated service badge label Mar 5, 2019

chris48s mentioned this pull request Mar 16, 2019

refactor [githubreleasedate] #3206

Merged

This was referenced Apr 10, 2019

Remove 'v' prefix from version StylishThemes/Wikipedia-Dark#59

Closed

Version / Github / Tag badge shows incorrect latest SemVer release #3080

Closed

chris48s mentioned this pull request Apr 14, 2019

Help wanted to finish refactoring services 🏆 #2863

Closed

paulmelnikow closed this Apr 17, 2019

chris48s mentioned this pull request Apr 19, 2019

refactor [JenkinsBuild] #3302

Merged

chris48s mentioned this pull request Jun 8, 2019

Stop calling variables "which"; affects [chocolatey codeclimate conda crates debug gem github mozillaobservatory nuget powershellgallery pub resharper vaadindirectory] #3495

Merged

chris48s mentioned this pull request Jun 25, 2019

move [githubtag] and [githubrelease] under /v, move variants to query params #3610

Merged

This was referenced Dec 15, 2019

Add documentation for designing badge URLs #4435

Merged

Convert vpre routes to include_prereleases param in [bower chocolatey clojars packagist powershellgallery pub resharper] #4436

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GitHubRelease] Get releases using semver (similar logic to tags) #3144

[GitHubRelease] Get releases using semver (similar logic to tags) #3144

RedSparr0w commented Mar 3, 2019

shields-ci commented Mar 3, 2019

paulmelnikow commented Mar 5, 2019

RedSparr0w commented Mar 5, 2019 •

edited

Loading

paulmelnikow commented Mar 5, 2019

chris48s commented Mar 5, 2019

chris48s commented Mar 5, 2019

RedSparr0w commented Mar 5, 2019

RedSparr0w commented Mar 5, 2019

paulmelnikow commented Mar 6, 2019

chris48s commented Mar 6, 2019

paulmelnikow commented Mar 7, 2019

paulmelnikow commented Mar 7, 2019

chris48s commented Mar 7, 2019

paulmelnikow commented Mar 7, 2019

chris48s commented Mar 10, 2019

paulmelnikow commented Mar 10, 2019

chris48s commented Mar 10, 2019

paulmelnikow commented Mar 10, 2019

paulmelnikow commented Apr 17, 2019

[GitHubRelease] Get releases using semver (similar logic to tags) #3144

[GitHubRelease] Get releases using semver (similar logic to tags) #3144

Conversation

RedSparr0w commented Mar 3, 2019

shields-ci commented Mar 3, 2019

paulmelnikow commented Mar 5, 2019

RedSparr0w commented Mar 5, 2019 • edited Loading

paulmelnikow commented Mar 5, 2019

chris48s commented Mar 5, 2019

chris48s commented Mar 5, 2019

RedSparr0w commented Mar 5, 2019

RedSparr0w commented Mar 5, 2019

paulmelnikow commented Mar 6, 2019

chris48s commented Mar 6, 2019

paulmelnikow commented Mar 7, 2019

paulmelnikow commented Mar 7, 2019

chris48s commented Mar 7, 2019

paulmelnikow commented Mar 7, 2019

chris48s commented Mar 10, 2019

paulmelnikow commented Mar 10, 2019

chris48s commented Mar 10, 2019

paulmelnikow commented Mar 10, 2019

paulmelnikow commented Apr 17, 2019

RedSparr0w commented Mar 5, 2019 •

edited

Loading