-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add [Weblate] badges #6677
Add [Weblate] badges #6677
Conversation
|
Thank you for the PR! Haven't gone through the code in detail yet but a few comments/requests:
|
Admittedly, the rest were just convenient data-points to grab while I was doing the above. If it's preferred, I can remove them?
I'll define them just so we're on the same page, but I appreciate your point.
That would be possible with the public instance. I think overall it might be hard though since there are many hosts for Weblate. Even I host my own Weblate instance for my projects. A non-exhaustive list can be found on the Discover page on Weblate's official website.
Ahh, thanks for that! Will push this in a moment and update the screenshot.
I've already done this a bit, but I can encapsulate more of it, so it's more reusable. I'll do this tonight/tomorrow. Edit: We have one called const isMetricWithText = text => {
const pattern = `^([1-9][0-9]*[kMGTPEZY]?|[1-9]\\.[1-9][kMGTPEZY]) ${text}$`
const regexp = new RegExp(pattern)
return withRegex(regexp)
}
const isMetricOpenIssues = isMetricWithText('open') Unfortunately, JavaScript doesn't support |
Sure! It's a bit of a balancing act of course as we don't want to bloat the helper files with single-use functions for bespoke cases, but we also don't want to reinvent the wheel in the individual service classes/tester files either. I suspect we've probably got a few cases of such duplication, and if you see opportunities for encapsulating such a helper (regardless of whether that's with these proposed badges or things already merged) definitely feel free to suggest improvements/submit a PR. I do think that'd probably be best handled via a separate PR which we could land first and this PR could then grab and utilize |
Let me know if I'm off base but so far this sounds similar to some other badges we support, where there's a defacto public instance typically a SaaS-style offering but also comes in a downloadable/self-hostable form too. In these cases we add support to the service class for authenticated requests, configure our production environment with the auth info from an account we own against the main public instance, and then users that need to access an instance other than the main one and/or with their own auth will run their own self-hosted instance of the Shields server |
Sounds valid for this one as well. I'll check how other services are configured and update this one to follow suit. |
Yikes! That's a good call. We have a ceiling of ~3-4 seconds total for the entire round trip to get the badges response back to users, otherwise they timeout and don't render in GitHub |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again @SethFalco! Have taken a first pass through this and left some specific feedback inline below.
I'm not sure where we landed as far as which badges make sense to keep (i.e. which badges users will want/consume), so if some of the files/badges have gone out of scope feel free to remove those files and disregard feedback in those files
services/weblate/weblate-project-translated-percentage.service.js
Outdated
Show resolved
Hide resolved
services/weblate/weblate-project-translated-percentage.tester.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Caleb Cartwright <[email protected]>
This pull request introduces 1 alert when merging e9b517b into 08b8153 - view on LGTM.com new alerts:
|
Thank you very much for giving such thorough feedback! I'll be sure to keep this in mind for future pull requests as well. I believe I've adhered to all feedback. I've resolved the ones that I'm pretty confident I've done exactly as asked. The only change I haven't done is the JSDoc one, thought I'd wait for further opinion. I've also updated the screenshot in the PR description.*
The only ones I'm pretty adamant on keeping are the following, as I have hopes to use these 2 myself:
I've looked over the badges and removed 3 which, I believe, would have little to no use-case:
I've left the rest in, as I'd imagine there could be a use-case for them, however you're welcome to suggest otherwise. I have no strong feelings on the matter. Edit: Sorry, I didn't think to edit the PR title until after I made all the changes. ^-^' Might want to rerun CI. |
No worries! I genuinely don't know enough about the service/what's meaningful for users so I'm not prescribing anything one way or another, just wasn't sure if we'd done any passes through
Have updated and restarted the ci jobs accordingly. The test matcher is smart enough (most of the time) to be able to just use the service name when you want to run all the tests like we do here, but we still give the individual testers more specific names so that we also have the option of running a subset of the tests, i.e. if in the future we make a change to the user translation badge then we can run just those tests vs. having to run the entire weblate suite. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent work, thank you!
@calebcartwright Sorry to pester, just a few questions. I just realized I forgot to add authentication on the PR. Could I just check, should I only add support for an API key with the official instance, or could we try and support other instances? For example, using the key Then we could just take the domain (https://i18n.example.org) to get the respective API key if it's defined:
If there's a better way to do it, that'd be nicer. Meanwhile, I find it's very unlikely someone will define keys for I also learned in the PR to Wikiapiary that it's possible to define enums in the URL pattern. 🤔 I can also abstract the entity count one similarly: This will reduce the total badge count (only 4 badges in total), and clean up the code/tests. Could be nice to leave in some of those extra options as well, since they'll just be part of the same badge? (If required, I can do so while preserving backward compatibility as well I think.) |
No worries at all, don't hesitate to ask any questions!
Yes indeed. To be honest we need to adjust a few of these routes anyway to align with our badge route guidelines. In this case the badges are so new (and unused) that we don't need to bother with a redirector, so let's just correct the routes to what we want them to be and try to get it completed quickly. By any chance do you think you'll have bandwidth to get this done soon? |
No problem! Sorry about that. Tomorrow I'll make a PR with the following changes:
After that PR is merged, I'll look at authentication again. |
That one slipped my mind as well. That's a lower priority compared to adjusting the routes, but we should try to get this plugged in fairly quickly too given the low rate limit.
I'm not sure I follow what you're asking so will provide a broad answer to see if that covers, but please let me know if you still have any outstanding questions. There are two core patterns we support for incorporating auth info in the requests we make to the upstream services
The second one is something we tend to prefer to avoid as it's only viable in cases where the upstream service makes it possible to generate a strictly read-only token. Our codecov badge is one example of this where the badge route accepts a The other cases follow that first pattern, where support for a secret is added to the server (currently supported secrets enumerated here) which allows any deployment of the badge server to define a value for that secret. For the Shields.io service (which is essentially just the main deployment of the Shields server), we deploy with values provided for those various secrets using accounts that we own and manage on those upstream platforms (more details here: https://github.com/badges/shields/blob/master/doc/production-hosting.md). Our secrets-setting mechanism doesn't support a variable number of secrets for multiple instances/tenants of a given service; there is a 1:1 mapping between a target service/platform and the corresponding auth secret for that service. This is just a basic reflection of practicality, as it wouldn't be realistic nor necessary for that matter for the main Shields.io instance to be able to authenticate with an open ended number of instances of a given service/tool/platform. That pattern needs to apply here as well: there should only be one secret defined for weblate. It sounds like there's a de facto public instance, so once the server code is updated to support the new weblate secret then one of us on the Shields maintainer team will create an account and generate the corresponding values, which we'll then plug in to the Shields.io runtime. |
This adds Weblate badges to the repository.
Weblate is a free and open-source crowd translation platform.
Currently, it adds 8 badges:
Examples:

I might make more badges later, but for now, I wanted to get these right.
Related