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

VAST-294 fix: upstream tag sync #3

Merged
merged 21 commits into from
Jan 9, 2025
Merged

Conversation

domcyrus
Copy link
Collaborator

@domcyrus domcyrus commented Jan 6, 2025

This PR is fixes the following issues:

  • action.yml broken yaml syntax on multiline description
  • git push authentication issue
  • simplify PR creation without conflict handling (conflict handling should be done during normal PR processing)
  • introduce simple retry mechanism on all github API calls
  • update README.md

upstream-tag-sync/action.yml Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
@domcyrus domcyrus requested a review from devnoname120 January 8, 2025 09:28
Copy link
Member

@devnoname120 devnoname120 left a 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!

upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
upstream-tag-sync/src/index.ts Outdated Show resolved Hide resolved
@domcyrus domcyrus requested a review from devnoname120 January 8, 2025 13:43
Copy link
Contributor

@RudolfSchreier RudolfSchreier left a 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!)

@domcyrus
Copy link
Collaborator Author

domcyrus commented Jan 8, 2025

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?

@domcyrus
Copy link
Collaborator Author

domcyrus commented Jan 8, 2025

@RudolfSchreier I've changed the retry mechanism to follow gh best practice using incremental backoff as last resort.

@RudolfSchreier
Copy link
Contributor

@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 :)

@RudolfSchreier RudolfSchreier removed their request for review January 9, 2025 09:33
@domcyrus domcyrus merged commit 551eb03 into main Jan 9, 2025
@domcyrus domcyrus deleted the VAST-294-upstream-tag-sync branch January 9, 2025 10:07
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.

3 participants