-
Notifications
You must be signed in to change notification settings - Fork 10
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 automatic module updates. Closes filecoin-station/roadmap#53 #316
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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.
Great work!
I did a quick first pass of reviewing the changes and would like to discuss a few places that may need further work.
`https://api.github.com/repos/${repo}/releases/latest`, | ||
{ | ||
headers: { | ||
...(authorization ? { authorization } : {}) |
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.
Do you have any concerns about calling GitHub API anonymously? If there are more clients in the same network calling their API, then we will start seeing "rate limit exceeded" errors.
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.
You can make unauthenticated requests if you are only fetching public data. Unauthenticated requests are associated with the originating IP address, not with the user or application that made the request.
The primary rate limit for unauthenticated requests is 60 requests per hour.
Iiuc, this can only happen when there's many core instances on the same IP - which (for now) we don't want anyway.
I think otherwise we have to host our own updates api endpoint.
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.
Iiuc, this can only happen when there's many core instances on the same IP - which (for now) we don't want anyway.
Fair enough 👍🏻
So, what happens when Station Core hits this rate-limiting error? Is this something we should be concerned about?
Example: will it retry the requests and thus make the situation even worse?
I think otherwise we have to host our own updates api endpoint.
I like that idea - the endpoint can send authenticated requests to GitHub, return cache-control (e.g. 1 minute?) and then we can use Cloudflare to shield us from too many requests.
Having written that, I think we don't need to add this extra complexity yet.
I am happy to keep things simple and use your current approach as long as we understand what will happen when we start hitting the rate limit errors, and we are okay with that.
…add/automatic-module-updates
This reverts commit 6c4cfb6.
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.
Great progress! I have a few more comments to discuss, but the PR is almost ready for landing.
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.
I think the current version is good enough 👍🏻
There are two unresolved discussion threads; I'll leave it up to you to decide if they offer any more worthwhile suggestions.
Closes CheckerNetwork/roadmap#53