Decide on what we should call "base url" parameters
Over the last 10+ years we have accumulated a lot of code written by a lot of different people. In general, I think we've done a reasonable job of enforcing relatively consistent code styles and naming conventions, via documentation and/or lint rules, but there are obviously some exceptions.
One significant "blind spot" where we have a lot of variance is what we call "base url" type parameters.
Related to #10805
We've got a bunch of services that call this param server
https://github.com/badges/shields/blob/41d072e1c93e997e8ac5f9c31cb763e351800bdb/services/drone/drone-build.service.js#L39-L42
Some that call it instance
https://github.com/badges/shields/blob/41d072e1c93e997e8ac5f9c31cb763e351800bdb/services/obs/obs.service.js#L40
baseUrl or base_url is quite common
https://github.com/badges/shields/blob/41d072e1c93e997e8ac5f9c31cb763e351800bdb/services/bugzilla/bugzilla.service.js#L40-L45
..and then some services include the name of the service e.g:
https://github.com/badges/shields/blob/41d072e1c93e997e8ac5f9c31cb763e351800bdb/services/revolt/revolt.service.js#L43-L46 https://github.com/badges/shields/blob/41d072e1c93e997e8ac5f9c31cb763e351800bdb/services/gitlab/gitlab-forks.service.js#L35-L38
Fundamentally, all of these describe the exact same concept. I think there is value in making this horizontally consistently across badges and I think the reasons why we have a lot of variance here is not down to trying to maintain consistency with upstream terminology.
There are a couple of ways we can change the names of query params without making a breaking change. One way we can do it is with redirects. Another would be to write the queryParamSchemas to accept both formats but only document one for the services where we want to fix this. We don't have loads of these, but I probably wouldn't want to do all of them in one PR. It would be nice to gradually work towards fixing this though.
I suggest we:
- Decide what we are going to call this concept
- Document it
- Maybe make some kind of a lint/danger rule to try and catch this, if possible?
- At least get to a stage where everything we add from now onwards matches this convention
- Bring existing services into line with the convention
IMO, we should pick a generic term: "server", "instance" or "baseUrl" (i.e: we should not have the service name in the param name). Beyond that, I don't really have a super strong opinion on which name we pick.
Just thinking about this a bit more (particularly in the context of https://github.com/badges/shields/pull/10792 and https://github.com/badges/shields/issues/10805 ) I think baseUrl makes more sense as a name for this concept than server or instance.
In the case where the URL includes an endpoint (i.e: your thing lives at https://bugs.eclipse.org/bugs or https://issues.apache.org/jira or whatever) baseUrl feels more right than server or instance.
Should we force https only? I like more the idea offered at #2637 where we would still support http but make it harder to use, maybe add a warning in docs near this option.
Regarding the name, i am for something like endpointUrl but i don't object baseUrl i think it still makes sense.
I would go with baseUrl but I'm also fine with endpointUrl.
But as already mentioned in https://github.com/badges/shields/pull/10792, Url would only fit if we include the scheme inside the parameter (referring to https://github.com/badges/shields/issues/10805).
would only fit if we include the scheme inside the parameter
Yeah :+1: so to summarise https://github.com/badges/shields/issues/10805
- This param should always be a full URL including protocol://
- We should bring the two exceptions to this (matrix, mastodon) into line
- If we want to enforce HTTPS, we can do it with validation in the
queryParamschemaJoi.string().uri({ scheme: ['https'] })
Just coming back to this: Another slightly different way to enforce HTTPS at the validation layer is to write
Joi.string()
.uri({ scheme: ['http', 'https'] })
.custom(url =>
url.startsWith('http') ? url.replace(/^http:\/\//, 'https://') : url,
)
That variant accepts https://example.com or http://example.com and then replaces http:// with https:// as part of the validation, so by the time we see the value it is already https://
The logistics and value of this is another issue, and lets continue that in https://github.com/badges/shields/issues/2637
Point is.. if we want to accept a URL and enforce HTTPS, there's ways of doing that at the validation layer without telling the user to split their URL up into parts.
That variant accepts https://example.com or http://example.com and then replaces http:// with https:// as part of the validation, so by the time we see the value it is already https://
Would that be transparent enough for the user?
If I create a badge with the service http://test.com but internally shields calls https://test.com without letting him know, that could be a problem right?
I think i'm for baseUrl as suggested by @cyb3rko .
@chris48s I would like a confirmation before i move forward and make a PR to document this, and hopefully some automation.
Once documented I think I should open a new issue that summarizes all shields that needs to be updated to track our progress.
I'm down with baseUrl 👍
Will go ahead and close this as a decision was reached. We can leave #11185 open as a follow-up task.