Add API-based support for [GitLab] badges, add new GitLab Tag badge #6988
Conversation
calebcartwright
added
service-badge
| export default class GitLabBase extends BaseJsonService { | ||
| static auth = { | ||
| passKey: 'gitlab_token', | ||
| serviceKey: 'gitlab', |
There was a problem hiding this comment.
We'll constrain the allowed origins for Shields.io to gitlab.com, but need this to be flexible for other self-hosters which is why the key is used here instead of hardcoding the authorizedOrigins value to gitlab.com
|
There was a problem hiding this comment.
Nice work on tackling this. One question here. The rest are inline on the diff.
Have already setup the account and token, will plug them into the prod env before deploying
Do we also need to set a token for staging and/or CI or will the anonymous rate limit be sufficient for those environments?
I was thinking anonymous will be fine. With a 2k per minute limit I don't anticipate any issues in staging nor CI |
Right, yeah that is pretty generous and I think that should cover the custom |
calebcartwright
force-pushed
the
gitlab
branch
from
87ced2d
to
b96f597
Compare
| static render({ version, sort }) { | ||
| return { | ||
| message: addv(version), | ||
| color: sort === 'semver' ? versionColor(version) : 'blue', | ||
| } | ||
| } |
There was a problem hiding this comment.
This is mostly copy/pasta from the corresponding GitHub service for consistency, but probably worth a moment of discussion given the change (the introduction of the v prefix).
There's been recurring discussion on this in general, and we certainly want to keep our standard posture (refs #3200, #1181, and #6135). However, we have previously expressed an openness to consider no longer inserting the v prefix on the GitHub Tag badges (e.g. #3200 (comment)).
Accordingly I actually think the message should be adjusted here to just display version, sans the addv call, but wanted to see what others thought first
There was a problem hiding this comment.
I do see where you're coming from and this is a good point to raise.
Our starting point for this is that (pretty much?) all of our badges add the v prefix by default. If we were to decide to allow users to disable it, the pattern is going to be that everything appends v by default but then you can pass ?addv=false or whatever to disable it.
Given that, I think it probably makes most sense to set GitLab up the same way as everything else to start with (i.e: with addv()). If we don't we will either end up saying "all the version badges append v by default and you can pass ?addv=false, except GitLab which is the other way round" or we have to make a behaviour change for GitLab in future to make it consistent.
There was a problem hiding this comment.
If we were to decide to allow users to disable it, the pattern is going to be that everything appends v by default but then you can pass ?addv=false or whatever to disable it.
to clarify, i'm not proposing we expose this level of control to the users via a query param or other means. it's strictly a question of what Shields does internally, either always injecting a leading v or not. I don't think this is something we can change our minds on down the road, or at least it's not something I think we should bank on having the liberty to do, so I feel like we should be confident and comfortable with whatever option we select.
in hindsight the tag badge is somewhat of a challenge to incorporate as the example part of what is largely centered around the general auth story for gitlab, given all the associated nuances
There was a problem hiding this comment.
Yes I think once we pick an approach here we're basically stuck with it. There isn't a single unambiguously correct choice. I think I'm still in favour of addv() just because it is what we do on all the other version badges (which this is https://github.com/badges/shields/pull/6988/files#diff-36f70802cce1e0dcc5c595f7913c44e5bce18d58c8357d8567b582399c7fcdb1R22 ) and it is what we'd be doing if we were using renderVersionBadge() but if you (or other @badges/shields-maintainers ) reckon there's a strong case for it here I'm not wedded to that opinion. Tags are slightly different and as I say, neither option is unambiguously correct.
There was a problem hiding this comment.
I would tend to agree with the argument "let's be consistent with what the GitHub badges do".
There was a problem hiding this comment.
Fair enough, I can go with that. One notable piece is that at past ops meetings we've discussed the need to brand our new GitLab badges with somewhat of a "beta" tag given the outstanding uncertainty around the auth story once we start to increase the scale of badges we offer and serve at runtime.
I'll post a note back in the main issue to that effect, which could in turn give us some leeway to revisit the message on this badge too.
| static category = 'version' | ||
|
|
||
| static route = { | ||
| base: 'gitlab/v/tag', |
There was a problem hiding this comment.
on the recurring theme of consistency, both with github and at large, i've updated the route to match the github tag badge route. however, i realize i'm not completely convinced this aligns with the service/noun/parameters badge route pattern enumerated in our docs.
we've also got other github routes that have the v and noun order reversed, e.g. github/v/tag/... vs github/package-json/v/...
i think this is probably the best bet to provide the highest degree of consistency, but also feel like maybe the docs could use more clarity if this is a common exception to the main rule?
There was a problem hiding this comment.
Agree with your decision here 👍
There was a problem hiding this comment.
Pressing the big green tick on this as if we stick with addv() it is good to go, but happy to re-:+1: it if we change our minds on #6988 (comment)
calebcartwright
force-pushed
the
gitlab
branch
from
c019a1f
to
1bda9c9
Compare
|
Nuts, should've just let ranger add a merge commit but did a rebase instead. One last call for a 👍 whenever anyone gets a chance 😄 |
Closes #541, refs/implements the tag half of #4867. I've opted to use the close keyword on #541 as I feel that with the authenticated supported added here, and accompanying rate limit increase, we can now move forward with other, specific badge requests as needed following our standard badge intake flow.
Have already setup the account and token, will plug them into the prod env before deploying and will also share the relevant account info with the rest of the ops team.