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

Use new ArtifactHub cached API #217

Merged
merged 16 commits into from
May 9, 2023
Merged

Use new ArtifactHub cached API #217

merged 16 commits into from
May 9, 2023

Conversation

rbren
Copy link
Contributor

@rbren rbren commented May 8, 2023

This PR fixes issues w/ ArtifactHub rate limiting by using the new cache API AH is providing for Nova

Checklist

  • I have signed the CLA
  • I have updated/added any relevant documentation

Description

What's the goal of this PR?

Move to the new cached API

What changes did you make?

Added and utilized a new "cached client"

I also removed some unused fields from the old struct, to help ensure that we're getting all the data we really need

What alternative solution should we consider, if any?

  • Make the old client usable via a flag?
  • Cache the response locally to a file

@rbren rbren requested a review from bbensky as a code owner May 8, 2023 12:50
@fairwinds-insights
Copy link

Fairwinds Insights CI Report

View the Full Report

✅ No new Action Items detected!

@rbren rbren mentioned this pull request May 8, 2023
r.Header.Add("accept", "application/json")
r.Header.Set("User-Agent", ac.UserAgent)
var response *http.Response
for attempt := 1; attempt <= 5; attempt++ {
Copy link

Choose a reason for hiding this comment

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

It'd be great if you could use some sort of exponential backoff (like https://github.com/cenkalti/backoff) or introduce a pause instead of retrying immediately a few times when something goes wrong. In some situations (like when encountering rate limit errors), this may make things even worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! I removed the retry logic entirely. I think it's an artifact from when we originally queried the repositories themselves, which were very flaky

@rbren rbren changed the title [WIP] Use new ArtifactHub cached API Use new ArtifactHub cached API May 9, 2023
@rbren rbren merged commit 7582f5b into master May 9, 2023
@rbren rbren deleted the rb/ah-cache branch May 9, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants