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

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

arnaud-dezandee
Copy link
Contributor

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 d5b9a27

@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

@arnaud-dezandee arnaud-dezandee force-pushed the feat/terraform-downloads branch from e92406e to d5b9a27 Compare January 12, 2025 18:34
@arnaud-dezandee
Copy link
Contributor Author

Hi @chris48s, I agreed with all your comments and made the necessary adjustments 👍
Also, I don't know the current rate limit value of the Terraform registry. I guess an hour should be fine

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.

smashed it

@chris48s chris48s added this pull request to the merge queue Jan 13, 2025
Merged via the queue into badges:master with commit 4ba06c0 Jan 13, 2025
23 checks passed
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