move [githubtag] and [githubrelease] under /v, move variants to query params #3610
Conversation
- move github /tag and /release under /v - both sort by date as default - specify sort=date/semver, include_prereleases as query params
chris48s
changed the title
|
I've now split everything back out into tag and release again and rebased the commits so its a bit easier to review. |
| const kvpairs = Object.assign( | ||
| ...releases.map(release => ({ [release.tag_name]: release.prerelease })) | ||
| ) | ||
| return { tag_name: latestRelease, prerelease: kvpairs[latestRelease] } |
There was a problem hiding this comment.
Nice 👍
Side note, but it might be nice one day if we could have the latest helper return the prerelease indicator along with the latest version it found (something like return { version, prerelease: ... } instead of return version) to avoid the need for callers to have to make that check themselves
| base: 'github/tag-date', | ||
| pattern: ':user/:repo', | ||
| }, | ||
| transformPath: ({ user, repo }) => `/github/v/tag/${user}/${repo}`, |
There was a problem hiding this comment.
Just want to confirm that we don't need the include_prereleases query param here correct? If I'm following the original code correctly, tag-date excluded pre-releases so this redirect maintains consistency.
There was a problem hiding this comment.
On the /releases endpoint, the include_prereleases is meaningful regardless of whether we're sorting by date or semver because there is an "prerelease" boolean.
On the /tag endpoint, that doesn't exist - we've just got a string - so include_prereleases is only meaningful if sort=semver (because we're inferring whether a release is a pre-release ot not based on parsing the string as a semantic version). If sort=date, the output is exactly the same regardless of whether you pass include_prereleases or not. That's also why there isn't an example for that case.
Instead of /v/tag/:user/:repo?sort=date and /v/tag/:user/:repo?sort=date&include_prereleases we could set it so that /v/tag/:user/:repo?sort=date&include_prereleases throws an error (e.g: "invalid parameter") to make this clearer?
There was a problem hiding this comment.
Thanks that helps!
I think I'd be inclined to keep the implementation as-is (not throwing an error) given that it's just unused/the default behavior anyway. I could envision someone down the road asking for the most recent, non-prerelease tag by date (not entirely sure what the use case would be, but could definitely see it being requested anyway 😄)
In order to support that, we'd need users to be able to use the include_prereleases param to indicate their preference, and could then use it with a slightly updated definition for getLatestTag:
static getLatestTag({ tags, sort, includePrereleases }) {
if (sort === 'semver') {
return latest(tags.map(tag => tag.name), { pre: includePrereleases })
}
if (includePrereleases) {
return tags[0].name
}
// use semver package to find first non-prerelease tag
const tag = tags.find(t => !semver.prerelease(t.name))
if (!tag) {
throw new NotFound({ prettyMessage: 'No release tags found' })
}
return tag.name
}There was a problem hiding this comment.
Also, does the GitHub API for tags automatically sort tags by semver?
When I take a look at the most recent tags for pytest: https://github.com/pytest-dev/pytest/tags, it seems pretty clear that v4.6.4 was published ~3 hours after the v5.0.0 tag. However, the API response for pytest tags appears to have them sorted by semver:
[
{
"name": "5.0.0",
"zipball_url": "https://api.github.com/repos/pytest-dev/pytest/zipball/5.0.0",
"tarball_url": "https://api.github.com/repos/pytest-dev/pytest/tarball/5.0.0",
"commit": {
"sha": "58bfc7736fc4f88eca669157822e00715c67a9bf",
"url": "https://api.github.com/repos/pytest-dev/pytest/commits/58bfc7736fc4f88eca669157822e00715c67a9bf"
},
"node_id": "MDM6UmVmMzc0ODk1MjU6NS4wLjA="
},
{
"name": "4.6.4",
"zipball_url": "https://api.github.com/repos/pytest-dev/pytest/zipball/4.6.4",
"tarball_url": "https://api.github.com/repos/pytest-dev/pytest/tarball/4.6.4",
"commit": {
"sha": "d3549df5b94137453b832345a9b8074f518731b0",
"url": "https://api.github.com/repos/pytest-dev/pytest/commits/d3549df5b94137453b832345a9b8074f518731b0"
},
"node_id": "MDM6UmVmMzc0ODk1MjU6NC42LjQ="
},
{
"name": "4.6.3",
"zipball_url": "https://api.github.com/repos/pytest-dev/pytest/zipball/4.6.3",
"tarball_url": "https://api.github.com/repos/pytest-dev/pytest/tarball/4.6.3",
"commit": {
"sha": "b8e65d03bf563d5de3a285af187fd00a2f202e5e",
"url": "https://api.github.com/repos/pytest-dev/pytest/commits/b8e65d03bf563d5de3a285af187fd00a2f202e5e"
},
"node_id": "MDM6UmVmMzc0ODk1MjU6NC42LjM="
},
...
]And accordingly our badges show v5.0.0 for the latest tag by date:
prod
pr review app
There was a problem hiding this comment.
Well, that's an exciting development, isn't it..
Turns out the GitHub API sorts tags... in (reverse) alphabetical order 😮
That's not in the API docs anywhere, but after reading that on stackoverflow (and thinking "wtf?") I set up a test repo just to prove it to myself: https://api.github.com/repos/chris48s/tags-test/tags
a24edac 2019-06-29 22:06:10 +0100 (tag: e)
4ca5cd6 2019-06-29 21:59:53 +0100 (tag: f)
bfa97ef 2019-06-29 21:54:52 +0100 (tag: d)
b3614ac 2019-06-29 21:51:20 +0100 (tag: 1)
b645016 2019-06-29 21:51:12 +0100 (tag: 2)
3309766 2019-06-29 21:49:47 +0100 (tag: c)
8d7c78b 2019-06-29 21:49:42 +0100 (tag: b)
5c5a8d3 2019-06-29 21:49:05 +0100 (tag: a)
Additionally, the tags page sorts them differently to the API, but also seemingly not chronologically either: https://github.com/chris48s/tags-test/tags
Of course, thinking it through a bit more, @RedSparr0w is actually at least 4 months ahead of me on this: #3080 (comment) if not more 😆
Not really sure what to do with this now.. let me have a think about it :D
There was a problem hiding this comment.
Having done a bit of reading, the only way to order tags by date in a performant way is to use the v4/graphql API. At the moment, we don't have any badges that use the v4 API, but API calls for new GitHub features (e.g: package registry, dependency graph, etc) are only being exposed via the v4 API so we'll need to get into it at some point.
Given that, we do need to look into the v4 API, maybe I'll put this on hold for the moment and use this as an excuse to have a poke at the v4 API and see what the implications would be for the token pool, etc. That might be the solution here, bit its also probably something we need to look into anyway.
There was a problem hiding this comment.
Sounds like a good plan to me
|
This was just posted to an old thread:
I just re-deployed the review app to http://shields-staging-pr-3610.herokuapp.com/. @Sopor can you give it a try to see if it works for you? |
@paulmelnikow I can't see any difference and i have tested them all but no |
Only the first 30 tags are shown currently, But as @chris48s said above, Not sure why that isn't showing as a proper release though, It doesn't even appear on the API On another note, is something wrong with the |
|
In the long run, switching over to the GraphQL API is a great idea. I've a little bit of experience querying GraphQL endpoints from "plain" apps (without a GraphQL framework) though I'm not sure what the best approach for Shields would be. I'd have to dig back in a bit. |
|
I haven't done anything on this in the last few days (I got a bit sucked into the JSDoc thing, which grew in complexity). I'm going to try and find a bit of time to get into this later in the week. |
|
Switching this to the GraphQl endpoint will produce the desired result in this case. For example, this query will return I did a bit of work on this last week. I've got a generic I haven't got too deep into it at this point, but I've played with it enough to understand that for Github specifically, the token pool layer is going to need to work differently for the v3 (REST) and v4 (GraphQl) APIs. I'm happy to continue plugging away at that as I have time, but its a bit involved so I need to try and carve out a decent chunk of time to work on it. Will update once I've got something worth reviewing. |
Is it a matter of using separate counters? We already do that for the |
|
By the way, which GraphQL fetch library are you using? The code I was using to make a simple GraphQL request is here: It's using this package: https://www.npmjs.com/package/graphql-request It looks like Lokka is another possibility. |
|
I'm not using any special library. I've just implemented it as a child class of It definitely needs seperate counters. That might be all it needs, but I've not got deep enough into it yet to have worked that out yet. I'll have a look at the code for |
|
having shaved the graphql yak, I've now finished this off. |
|
I'm not sure why the deployment is showing as outdated 🤔 will try to get the review app redeployed |
|
The new functionality looks to be providing the desired results for the repo @Sopor shared 👍 https://shields-staging-pr-3610.herokuapp.com/ https://shields-staging-pr-3610.herokuapp.com/github/v/tag/xbmc/xbmc The latest release (by date) is still showing 18.2, but I believe that is actually the correct result beceause there's a tag for 18.3, but no associated release |
| const queryParamSchema = Joi.object({ | ||
| include_prereleases: Joi.equal(''), | ||
| sort: Joi.string() | ||
| .valid('date', 'semver') |
There was a problem hiding this comment.
another wishlist item from me not directly related to this PR, but it'd be cool if the modal window for the query params could have a dropdown for cases like these like we do for route params
There was a problem hiding this comment.
This is looking great! Excited about all the issues this will resolve. I left a couple minor inline items, but looks about ready to go from my perspective.
I did have one question from using the test repo you set up (https://github.com/chris48s/tags-test/tags) in the review app. What's the expected latest tag by date for that repo?
Based on the timestamps you shared inline:
a24edac 2019-06-29 22:06:10 +0100 (tag: e)
4ca5cd6 2019-06-29 21:59:53 +0100 (tag: f)
bfa97ef 2019-06-29 21:54:52 +0100 (tag: d)
b3614ac 2019-06-29 21:51:20 +0100 (tag: 1)
b645016 2019-06-29 21:51:12 +0100 (tag: 2)
3309766 2019-06-29 21:49:47 +0100 (tag: c)
8d7c78b 2019-06-29 21:49:42 +0100 (tag: b)
5c5a8d3 2019-06-29 21:49:05 +0100 (tag: a)
I was expecting e, but when I go through the modal in the review app it's showing 2 as the latest tag by date
https://shields-staging-pr-3610.herokuapp.com/github/v/tag/chris48s/tags-test
|
Oh well spotted. Thanks. That is unexpected behaviour to me based on the description of query {
repository(owner: "chris48s", name: "tags-test") {
refs(refPrefix: "refs/tags/", first: 30, orderBy: {field: TAG_COMMIT_DATE, direction: DESC}) {
edges {
node {
name,
target {
...on Commit {
committedDate
}
}
}
}
}
}
}Yields the response: {
"data": {
"repository": {
"refs": {
"edges": [
{
"node": {
"name": "2",
"target": {
"committedDate": "2019-06-29T20:51:12Z"
}
}
},
{
"node": {
"name": "1",
"target": {
"committedDate": "2019-06-29T20:51:20Z"
}
}
},
{
"node": {
"name": "f",
"target": {
"committedDate": "2019-06-29T20:59:53Z"
}
}
},
{
"node": {
"name": "e",
"target": {
"committedDate": "2019-06-29T21:06:10Z"
}
}
},
{
"node": {
"name": "d",
"target": {
"committedDate": "2019-06-29T20:54:52Z"
}
}
},
{
"node": {
"name": "c",
"target": {
"committedDate": "2019-06-29T20:49:47Z"
}
}
},
{
"node": {
"name": "b",
"target": {
"committedDate": "2019-06-29T20:49:42Z"
}
}
},
{
"node": {
"name": "a",
"target": {
"committedDate": "2019-06-29T20:49:05Z"
}
}
}
]
}
}
}
}I'd expect this query to sort such that the tag object 'e' is first as it has the latest commit timestamp. I've raised a ticket with GH support about this by email. Leave it with me and I'll see what they say. I'll hold off on updating the timestamps until I see what the outcome of this is. |
|
Reply from GitHub support:
|
chris48s
force-pushed
the
github-version
branch
from
3e56783 to
96db672
Compare
|
I've not heard anything more from GH support on this since I raised it a couple of weeks ago. Given that if/when this gets fixed in the upstream API, there is no change we need to make to accommodate the fix, what do you think to just merging it? Although we have hit an edge case that isn't handled by the V4 API, I reckon the effect of merging this is probably net-positive |
I had a similar thought as well, so I'm good with that. My only hesitation was that I wasn't sure if there was an off chance they could/would make any changes to the v4 API to address this that we'd need to take into account (I wasn't sure if the v4 API was subject to changes at any time). Agreed on merging though so I'll give it a 👍 and then hopefully this edge case will get sorted automatically later on when they fix whatever is going on behind the scenes. |
|
As far as I'm aware, v4 should be considered a stable/production-ready API, but it would be fair to say that at this point it has limited adoption in comparison to v3. |
Excellent and agreed. I also came across this: https://developer.github.com/v4/breaking_changes/ which may be useful for us down the road as we expand our v4 usage. |
|
Cheers. This one has taken a long time to come together since we initially discussed it. Feels good to finally get it over the line :) |
I've been meaning to get into this for quite a while but strugged to carve out the time. This follows on from this (now quite old) discussion: #3144 (comment) about resolving the odd inconsistent behaviours of
/tagand/releasewhich have evolved over time.I've merged both the tag and release badges into a single version badge on
github/vwith URL params forinclude_tags,include_prereleasesandsort=date/sort=semverand redirects on the legacy routes for compatibility. This gives us consitent behaviour for tags and releases with sensible defaults.Note I've only looked at Github tag/release version. I haven't addressed the issue of
v/vpreon other badges here. That can be done in another PR.