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 [CurseForge] badges #9252

Merged
merged 10 commits into from
Aug 13, 2023
Merged

Add [CurseForge] badges #9252

merged 10 commits into from
Aug 13, 2023

Conversation

SandroHc
Copy link
Contributor

@SandroHc SandroHc commented Jun 10, 2023

Adds the following badges:

URL Example Result
/curseforge/dt/:projectId /curseforge/dt/238222 Downloads - /curseforge/dt/238222
/curseforge/game-versions/:projectId /curseforge/game-versions/238222 Game versions - /curseforge/game-versions/238222
/curseforge/v/:projectId /curseforge/v/238222 Latest version - /curseforge/v/238222

To generate the secret CURSEFORGE_API_KEY (yml: private.curseforge_api_key) please signup to the CurseForge Console with a Google account and create an organization, then go to the API keys page and copy the generated API key. I can provide the API key I used for my own testing if creating a new one is too much of an hassle.

Also added the following logos: (Removed, as the logos were already available in the Simple Icons library)

Logo Location Source
CurseForge logo /logo/curseforge.svg https://static-beta.curseforge.com/images/favicon.ico
Modrinth logo /logo/modrinth.svg https://modrinth.com/favicon.ico

@github-actions
Copy link
Contributor

github-actions bot commented Jun 10, 2023

Warnings
⚠️ This PR modified the server but none of its tests.
That's okay so long as it's refactoring existing code.
Messages
📖 ✨ Thanks for your contribution to Shields, @SandroHc!
📖

Thanks for contributing to our documentation. We ❤️ our documentarians!

Generated by 🚫 dangerJS against 2bfbfb4

@calebcartwright
Copy link
Member

Thank you for the PR, although functionally I feel like this duplicates #7499

Are there any material differences with this PR that you think should be considered over #7499, and/or do you think anything has changed or that we should be aware of wrt the signup process? The seeming complexities around that were largely what things have been blocked on for a while

@SandroHc
Copy link
Contributor Author

Hey, thanks for the reply!

Regarding #7499, the only difference are two extra endpoints ("v/:projectId" and "game-versions/:projectId") and the tests, which are trivial to implement once the API token issue gets sorted out. I am okay with closing this PR in favour of #7499; the extra features can be introduced later. Should have checked if there was any previous work done beforehand, so my fault.

@PyvesB
Copy link
Member

PyvesB commented Jun 12, 2023

We have a Google account for Shields.io that we could reuse here. Alternatively I've got a Forge account I'm be happy to share for this matter. #7499 links to a form, happy to fill that out and submit it, it doesn't look nearly as annoying than the Youtube form I had to deal with yesterday (#8605). :)

@SandroHc
Copy link
Contributor Author

@PyvesB, that form is actually outdated! The new way is much simpler and just requires signing up with a Google account an creating an organization; no manual review needed. Please see the description of this PR for instructions. :)

The only question is what is the request cap per token, as the documentation doesn't talk about rate limits.

Adds the following badges:
 - /curseforge/dt/:projectId (downloads)
 - /curseforge/game-versions/:projectId (game versions)
 - /curseforge/v/:projectId (version)

The following secret:
 - CURSEFORGE_API_KEY (yml: private.curseforge_api_key)
@PyvesB
Copy link
Member

PyvesB commented Jun 18, 2023

@PyvesB, that form is actually outdated! The new way is much simpler and just requires signing up with a Google account an creating an organization; no manual review needed. Please see the description of this PR for instructions. :)

Thanks, it indeed is easy to get an API key. Though it's a bit unfortunate that you can only have one key per org, we'll potentially have to use someone's personal account for CI.

The only question is what is the request cap per token, as the documentation doesn't talk about rate limits.

Do you think you could drop the Curse Forge team a line to figure this out? Hitting various API rate limits has been a recurring problem for Shields.io, we wouldn't want to introduce a new badge if a few months down the line badges are breaking for users due to an unforeseen limit being hit. :)

@SandroHc
Copy link
Contributor Author

Sent an email (as per this page) asking about the rate limits. Will give an update once I get a reply.

@SandroHc
Copy link
Contributor Author

@PyvesB, they replied back asking for the expected traffic. Could I add you to the conversation, so that you could answer those hard-hitting questions that I don't have the answers to?

@calebcartwright
Copy link
Member

@PyvesB, they replied back asking for the expected traffic. Could I add you to the conversation, so that you could answer those hard-hitting questions that I don't have the answers to?

Short answer is yes to plugging the Shields team into the thread, but will also say that it's rather hard for us to provide anything other than an uninformed guestimate with something like this.

With a platform used as widely and heavily like GitHub, with as large a user base, we can generate a lot of traffic in the delivery of badges (we peak somewhere in the 4k-5k requests/minute with GitHub). We've got other badges where we only make a handful of API requests per day (e.g. less than 100).

It's ultimately a factor of the number of users of the platform, and the subset of those that want badges displaying data from/about that platform

@calebcartwright calebcartwright added the service-badge Accepted and actionable changes, features, and bugs label Jun 19, 2023
@PyvesB
Copy link
Member

PyvesB commented Jul 1, 2023

For anyone following this PR: we are currently talking to the Curse Forge team. Please bear with us whilst we exchange information with them. :)

@github-actions
Copy link
Contributor

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

Copy link
Member

@PyvesB PyvesB left a comment

Choose a reason for hiding this comment

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

The CurseForge team has responded:

Please move forward with the integration as is, and if anything should come up - please reach out to us again.

As the immediate next step, I've reviewed the code. Thanks for the contribution @SandroHc, this looks like great work to me! A few small changes requested inline. :)

One issue is that I've not managed to add an API key for GitHub Actions, I get the following error:

Failed to add secret. Secret names can only contain alphanumeric characters ([a-z], [A-Z], [0-9]) or underscores (_). Spaces are not allowed. Must start with a letter ([a-z], [A-Z]) or underscores (_). 

Indeed, their secret contains characters not listed above. Any ideas @chris48s ?

config/local.template.yml Outdated Show resolved Hide resolved
services/modrinth/modrinth-downloads.service.js Outdated Show resolved Hide resolved
},
]

static defaultBadgeData = { label: 'downloads', namedLogo: 'curseforge' }
Copy link
Member

Choose a reason for hiding this comment

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

As per our guidelines, logos should not be included by default for non social style badges. Could you please remove the logo for all new badges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

There were a few leftovers, I went ahead and removed them myself to avoid another review cycle :)

@SandroHc
Copy link
Contributor Author

I am not sure what the lint CI step is complaining about. Both files are fine when I check them locally:

$ npx prettier --check "core/base-service/auth-helper.js"       
Checking formatting...
All matched files use Prettier code style!

$ npx prettier --check "services/curseforge/curseforge-base.js"
Checking formatting...
All matched files use Prettier code style!

@PyvesB
Copy link
Member

PyvesB commented Jul 27, 2023

We recently updated to a new major version of Prettier (#9357). Could you make sure you're based off of a recent-enough master revision, and that you've installed the latest Prettier (npm ci)?

@SandroHc
Copy link
Contributor Author

That fixed it. All checks are passing now!

@PyvesB
Copy link
Member

PyvesB commented Aug 7, 2023

In the meantime we've been setting up the tokens on our servers, so whilst we've been a bit slow on the review cycles since this was first opened, this should be fully operational as soon as we merge (pending Chris's last comment) and kick off a deploy. Thanks for the patience! 👍🏻

@SandroHc
Copy link
Contributor Author

All changes are done now! I have retested all three badges to make sure nothing broke in the few months since I opened this PR and everything is looking good.

@PyvesB PyvesB added this pull request to the merge queue Aug 13, 2023
Merged via the queue into badges:master with commit 96e9e13 Aug 13, 2023
20 checks passed
@PyvesB
Copy link
Member

PyvesB commented Aug 13, 2023

I've deployed this PR to staging and production. The new badges are happily working, for example:

Thanks again for working with us on this @SandroHc!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants