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

Decide on some conventions for allowing the user to specify the URL we should call #10805

Open
chris48s opened this issue Jan 10, 2025 · 0 comments
Labels
developer-experience Dev tooling, test framework, and CI

Comments

@chris48s
Copy link
Member

chris48s commented Jan 10, 2025

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 params for allowing the user to specify (part of) the URL we should call to get data for the badge.

This comes out of a conversation in #10792 (comment)
Also related to #10806

Underneath this, there are a couple of cases.

  1. The first is where the entire URL we're going to call is a param

So this is like the dynamic json/yaml/xml/toml badges or the endpoint badge
but there are also some others like
OSSLifeCycle or Python Version from pyproject.toml

  1. The second case is where part of the URL we're going to call is hard-coded, but the user can specify the "base url". There may or may not be a default. This is important for services which are distributed or self-hosted.

Within this, there are 2 patterns. By far the most common of these is that the param is a URL (including the protocol:// )

Some examples:

queryParam({
name: 'server',
example: 'https://teamcity.jetbrains.com',
}),

queryParam({
name: 'gitlab_url',
example: 'https://gitlab.com',
}),

queryParam({
name: 'baseUrl',
example: 'https://issues.apache.org/jira',
required: true,
}),

Then there is a second less common pattern where the "base URL" is just a domain (excluding the protocol)

I think the only two examples we have of this pattern are Matrix and Mastodon:

queryParam({
name: 'domain',
example: 'mastodon.social',
}),

queryParam({
name: 'server_fqdn',
example: 'matrix.org',
}),

If anyone knows of another example of this then let me know, but I think there are only 2 cases of this.

As alluded to in #10792 (comment) this only works for services where the thing lives on a domain or subdomain but can't live under a subdirectory. So what I mean by this is some services don't have to live at the root of a (sub-)domain. For example, a JIRA instance might live at a URL like https://jira.spring.io or a URL like https://issues.apache.org/jira . If you express this as "domain" or "fqdn", it doesn't account for the second of those cases.

In my view, we should:

  1. Document that when we accept a "base url" type param, it a full URL (including protocol://)
  2. Fix the two exceptions to this (matrix, mastodon)

Mainly because this is what the majority of cases do, and playing the hand we've got this is the easiest way to make this consistent.

There are a couple of ways we can change the documented query params but maintain compatibility with an older format. One way we can do it is with redirects. Another would be to write the queryParamSchemas to accept both formats but only document one. This shouldn't be a big deal to fix these two cases and maintain compatibility.

There is one argument put forward in https://github.com/badges/shields/pull/10792/files#r1904687991 about forcing HTTPS.

Personally, I'd say the way we do that is by making the validator we use on the query param

Joi.string().uri({ scheme: ['https'] })

rather than

Joi.string().uri({ scheme: ['http', 'https'] })

which is what we use at the moment

/**
* Joi validator that checks if a value is a URL
*
* TODO: This accepts URLs with query strings and fragments, which for some purposes should be rejected.
*
* @type {Joi}
*/
export const optionalUrl = Joi.string().uri({ scheme: ['http', 'https'] })

I think if we want to force HTTPS, that's the way to do it. We can declare a shared validator for a URL (accepting HTTPS only) and use that for new services if we want. I'd rather see us do this than tell the user to split their URL into parts for us.

@chris48s chris48s added the developer-experience Dev tooling, test framework, and CI label Jan 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

No branches or pull requests

1 participant