Add [GitlabPipeline] badge #2325
Conversation
Generated by 🚫 dangerJS |
|
Have labeled this a blocker, as I won't prioritize the work of continuing to rewrite #1838 until this is merged. |
| const queryParamsSchema = Joi.object({ | ||
| // TODO This accepts URLs with query strings and fragments, which should be | ||
| // rejected. | ||
| gitlab_url: Joi.string().uri({ scheme: ['https'] }), |
There was a problem hiding this comment.
In this case, maybe it would be easier to validate and then work out if we've got a queryString (or strip it away) by parsing into a URLObject instead of using Joi:
> const { URL } = require('url')
> const url = new URL('http://foo.bar/baz?foo=bar')
> url
URL {
href: 'http://foo.bar/baz?foo=bar',
origin: 'http://foo.bar',
protocol: 'http:',
username: '',
password: '',
host: 'foo.bar',
hostname: 'foo.bar',
port: '',
pathname: '/baz',
search: '?foo=bar',
searchParams: URLSearchParams { 'foo' => 'bar' },
hash: '' }There was a problem hiding this comment.
That could work… though it's a somewhat messy thing to replicate across other services that might use this. Adding some flags to the Joi function seems like a better way to handle it.
Checking for valid URLs is a good start. It doesn't feel important to get this perfect.
| // hosted gitlab. | ||
| query: { gitlab_url: 'https://gitlab.com' }, | ||
| namedParams: { user: 'GNOME', repo: 'pango' }, | ||
| query: { gitlab_url: 'https://gitlab.gnome.org' }, |
There was a problem hiding this comment.
It might not be directly related to the changes in this PR (I suspect it was introduced in #2308 ), but I noticed that the query gets passed through to the placeholder example but not the one with the params filled in:
There was a problem hiding this comment.
Indeed. I noticed that too: #2339
There was a problem hiding this comment.
Since this is approved why don't we merge this. It'll make #2339 easier to review. I probably should have just added that change here.
|
Yea… they did run, though in #2326 they got new names so I changed the branch protection config, and now GitHub is waiting for the new names. Merging should fix it.
|
This starts the rewrite of the dynamic badges. I've pulled into BaseService an initial version of the query param validation from #2325. I've extended from BaseJsonService to avoid duplicating the deserialization logic, though it means there is a bit of duplicated code among the three dynamic services. The way to unravel this would be to move the logic from `_requestJson` and friends from the base classes into functions so DynamicJson can inherit from BaseDynamic. Would that be worth it? This introduces a regression of #1446 for this badge. Close #2345
There's a lot of demand for the Gitlab badges (#541) and the PR has been lingering, so I thought I'd start off one of the simple ones as a new-style service. This one is SVG-based, so it shouldn't require the API-token logic which could use some more testing and will require us to create an app and configure it on our server.
We don't have any validation in place for
queryParams. Probably this should be added to BaseService, though for the time being I extracted a helper function.Thanks to @LVMBDV for getting this work started in #1838!