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.

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

Github tag not showing the correct value #2117

Closed
Sopor opened this issue Sep 25, 2018 · 7 comments
Closed

Github tag not showing the correct value #2117

Sopor opened this issue Sep 25, 2018 · 7 comments
Labels
question Support questions, usage questions, unconfirmed bugs, discussions, ideas service-badge New or updated service badge

Comments

@Sopor
Copy link

Sopor commented Sep 25, 2018

I have noticed that this
https://img.shields.io/github/tag/osmc/osmc.svg?label=OSMC (Vero 4K)
have been the same for a long time and when i check the project https://github.com/osmc/osmc/tags it is now at tag 2018.08-1. It seems that it can't handle the -. That is the only reason i can find out...

I also found out that this is also showing the wrong tag, 3.51b:
https://img.shields.io/github/tag/airdcpp/airgit.svg?label=AirDC
The correct tag should be 3.51

If i use the release it seems to show 3.51:
https://img.shields.io/github/release/airdcpp/airgit.svg?label=AirDC
But not all projects use Releases, only Tags.

@chris48s
Copy link
Member

I think both of these things are fundamentally because the badge sorts tags as semantic versions rather than chronologically.

  • On the first one, the - is causing those tags to be treated as a pre-releases. If you call https://img.shields.io/github/tag-pre/osmc/osmc.svg it does render because it includes pre-releases although I'm not really sure what the meaning of - is on a project which uses calendar versioning.
  • With the second one, I think 3.51b is sorting unexpectedly because 3.51b isn't a valid semantic version.

More generally, there's several strange things going on here..

  • We seem to be making the assumption that all GitHub tags have to be semantic versions, which is not true.
  • If we receive a version that isn't a valid semantic version (which definitely will happen sometimes, given the above) we're attempting to sort it anyway, producing unexpected results.
  • We don't document that in the help for the badge. FWIW, my initial expectation was that this would just show the chronologically most recent tag until I read the source code.
  • tag-pre exists but there isn't an example for it on the home page.
  • To include pre-releases we're inconsistently using /tag-pre for GH tags and /releases/all for GH releases.

There are probably a lot of users depending on the existing functionality (which assumes semver), but there are probably also some things we can do to make it clearer what is going on here or make things a bit more consistent. @RedSparr0w I'm conscious you did some work on this fairly recently to solve several other problems but I can't remember what they were. Do you have any thoughts on this one?

@chris48s chris48s added question Support questions, usage questions, unconfirmed bugs, discussions, ideas service-badge New or updated service badge labels Sep 27, 2018
@RedSparr0w
Copy link
Member

@RedSparr0w I'm conscious you did some work on this fairly recently to solve several other problems but I can't remember what they were

#1628 was recently merged which worked on the latest version function.

Do you have any thoughts on this one?

I'm really not quite sure what the best way to handle this would be.
maybe updating this line to something similar to the below should handle it:

  if (!pre){
    // remove pre-releases from array
    versions = versions.filter(function(version) {
-      return !(/\d+-\w+/).test(version);
+      return !(/\d+-?[a-z][\w.-]*$/).test(version);
    });
  }

But this may just be a band-aid fix as it obviously wont cover all cases, but i think it should be able to handle most cases.

To include pre-releases we're inconsistently using /tag-pre for GH tags and /releases/all for GH releases.

Good point, I hadn't realized that at the time. #1682 (comment)
what do you think would be the best way to make this consistent again without much disruption?

my initial expectation was that this would just show the chronologically most recent tag

Chronological order does seem to make the most sense,
From memory (I may be wrong) it originally wasn't done this way to remove pre-release tags and possibly some other reasons, or maybe just because the latest() function already existed.

@chris48s
Copy link
Member

#1628 was recently merged which worked on the latest version function.

That was the PR I was looking for - cheers for the refresher on all this.

what do you think would be the best way to make this consistent again without much disruption?

Looking through the other examples (-pre) is much more common. Only GitHub releases seems to use the /all suffix. I reckon we should pick /tag-pre/username/repo and /releases-pre/username/repo, modify the releases regex and document them both like that on the home page but allow the regex to optionally accept the "legacy" /all format for backwards-compatibility. This will avoid breaking for users who already use that format.

Chronological order does seem to make the most sense,

Looking through the linked issues in PR #1628, some people do expect semver ordering and want pre-releases to be excluded etc so that is useful functionality. It seems like there are 2 competing use cases.

We could usefully allow

  • /tag/username/repo.svg?sort=semver
  • /tag/username/repo.svg?sort=date

and

  • /releases/username/repo.svg?sort=semver
  • /releases/username/repo.svg?sort=date

If we retain semver as the default if no sort param is passed that avoids breaking for existing users who rely on that. I think doing it as a GET param probably makes more sense in this case than defining different routes, but we could stick them on different routes if we wanted.

@chris48s
Copy link
Member

chris48s commented Oct 8, 2018

I've just merged #2157 . Once that's deployed you'll be able to use /tag-date and that will just show the latest (chronological) tag. This will be more appropriate for projects which follow alternative versioning schemes. It'll show up on the home page once its deployed, but I'll try and remember to post in this issue and close it off when that happens. Cheers

@chris48s
Copy link
Member

This is now live so for projects that don't use SemVer you can use

to just show the most recent tag (by date).

@Sopor

This comment has been minimized.

@paulmelnikow

This comment has been minimized.

@badges badges locked as resolved and limited conversation to collaborators Jul 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
question Support questions, usage questions, unconfirmed bugs, discussions, ideas service-badge New or updated service badge
Projects
None yet
Development

No branches or pull requests

4 participants