-
Notifications
You must be signed in to change notification settings - Fork 0
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
VAST-294 fix: upstream tag sync #3
Conversation
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.
Some nits, looks good otherwise!
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.
Hi - I see that Paul has already added a lot of thorough input, so I will only add this big top-level question:
Why does this PR implement a retry mechanism manually (using hardcoded 1000ms and a handwritten backoff strategy) rather than rely on the mechanism provided by the API itself? See doc at https://docs.github.com/en/rest/using-the-rest-api/rate-limits-for-the-rest-api?apiVersion=2022-11-28. Especially the section "Best practices for using the REST API > Handle rate limit errors approriately" has a lot of guidance that this PR is not implementing, but could be. (The exponential retry is only recommended as a last resort, rather than the first cause of action!)
@RudolfSchreier Do you have some example code which follows the github blessed retry mechanism? |
@RudolfSchreier I've changed the retry mechanism to follow gh best practice using incremental backoff as last resort. |
I've synced on this with @roosnic1 yesterday and am also a bit swamped with work - please feel free to move forward on this without my required approval :) |
This PR is fixes the following issues:
action.yml
broken yaml syntax on multiline description