shields icon indicating copy to clipboard operation
shields copied to clipboard

Decide on what we should call "base url" parameters

Open chris48s opened this issue 11 months ago • 8 comments

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:

  1. Decide what we are going to call this concept
  2. Document it
  3. Maybe make some kind of a lint/danger rule to try and catch this, if possible?
  4. At least get to a stage where everything we add from now onwards matches this convention
  5. 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.

chris48s avatar Jan 10 '25 17:01 chris48s

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.

chris48s avatar Jan 10 '25 18:01 chris48s

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.

jNullj avatar Jan 10 '25 18:01 jNullj

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).

cyb3rko avatar Jan 10 '25 18:01 cyb3rko

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 queryParam schema Joi.string().uri({ scheme: ['https'] })

chris48s avatar Jan 10 '25 19:01 chris48s

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.

chris48s avatar Jan 12 '25 15:01 chris48s

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?

cyb3rko avatar Feb 21 '25 07:02 cyb3rko

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.

jNullj avatar May 13 '25 18:05 jNullj

I'm down with baseUrl 👍

chris48s avatar May 13 '25 20:05 chris48s

Will go ahead and close this as a decision was reached. We can leave #11185 open as a follow-up task.

PyvesB avatar Nov 02 '25 14:11 PyvesB