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

Add [Terraform] registry providers and modules downloads #10793

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

arnaud-dezandee
Copy link

Hi!

This PR adds Terraform Registry downloads summary support for:

  • providers
  • modules

It uses public API endpoints as documented here

Copy link
Contributor

github-actions bot commented Jan 8, 2025

Messages
📖 ✨ Thanks for your contribution to Shields, @arnaud-dezandee!

Generated by 🚫 dangerJS against e92406e

@chris48s chris48s added the service-badge New or updated service badge label Jan 10, 2025
Copy link
Contributor

🚀 Updated review app: https://pr-10793-badges-shields.fly.dev

Copy link
Member

@chris48s chris48s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few things, but this is solid stuff 👍

static category = 'downloads'

static route = {
base: 'terraform/module/downloads',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have a look over all the other downloads badges, we just use serviceName/:interval(dw|dm|dy)... or whatever. We can remove the /downloads part from the route base here.

static category = 'downloads'

static route = {
base: 'terraform/provider/downloads',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

..and here

Comment on lines +16 to +17
id: Joi.string().required(),
type: Joi.string().required(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we don't actually use the id or type values.

If so, we can remove these two from the schema. We don't need to validate values we don't use: We should only fail if values we actually rely on to render the badge are missing from the response or don't match the expected format.

@chris48s
Copy link
Member

One of the things it says on

https://developer.hashicorp.com/terraform/registry/api-docs#http-status-codes

is

Clients placing excessive load on the service might be rate-limited and receive a 429 code. This should be interpreted as a sign to slow down, and wait some time before retrying the request.

but I can't find the actual rate limits (as in how many request per minute/hour/whatever we are allowed to make) explicitly stated. Do you have any idea what kind of rate limit we're trying to stay under?

For a rate limited service where we just show a number of downloads (in this case, up to date/precise number is not super important), how about we set the badge images to be cached downstream for an hour with static _cacheLength = 3600

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge New or updated service badge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants