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

chore: upgrade octokit dependencies #2459

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

restfulhead
Copy link
Contributor

The main goal here is to upgrade @octokit/plugin-throttling to 5, so that the secondary rate limit wait time is increased, wich hopefully fixes #2458.

This required other dependencies to be upgraded. The resolutions for @octokit/plugin-rest-endpoint-methods is necessary, because without it, it pulls in 10.0.0 of @octokit/types which isn't compatible with the other libraries.

Change Type

Indicate the type of change your pull request is:

  • documentation
  • patch
  • minor
  • major

@restfulhead
Copy link
Contributor Author

restfulhead commented May 2, 2024

Update: I seem to have finally found a combination of the @octokit plugins that are compatible with each other. They don't make it easy.

Unfortunately I still have to add a resolution for @octokit/plugin-rest-endpoint-methods, because:

  • @octokit/rest (19.0.13) defines the dependency "@octokit/plugin-rest-endpoint-methods": "^7.1.2"
  • If the dependency would instead be "@octokit/plugin-rest-endpoint-methods": "~7.1.2" it would work
  • But because of ^, it installs @octokit/plugin-rest-endpoint-methods with 7.2.x
  • 7.2.x however depends on @octokit/types of type 10.0.0
  • All other dependencies however depend on version 9.x

@hipstersmoothie
Copy link
Collaborator

Would love to see this get in!

@dpotyralski
Copy link

@restfulhead I would be great to merge this in :) Hope the related PR helped a bit, but still seems some tests are failing.

@LittleGreenYoda42
Copy link
Contributor

@restfulhead please rebase your branch on main and the failing tests should turn green. Is there anything else missing to get this merged?

Have a great day.

The main goal here is to upgrade `@octokit/plugin-throttling` to `5`, so that the secondary rate limit wait time is increased, wich hopefully fixes intuit#2458. This required other dependencies to be upgraded. The `resolutions` for `@octokit/plugin-rest-endpoint-methods` is necessary, because without it it pulls in `10.0.0` of `@octokit/types` which isn't compatible with the other libraries.
Finally found a combination of @octokit versions that all use @octokit/types 9.x
@restfulhead
Copy link
Contributor Author

@LittleGreenYoda42 @dpotyralski Thanks, the rebase is done and the test build is now green. 🙌

Is there anything else missing to get this merged?

In general, it would probably be a good idea for someone else to manually test this change. Also note that I haven't tested any plugins and I'm not sure how isolated they are from the dependency changes here. I checked minor as the change type, but potentially this could be a breaking change and maybe it should be a major release?

@dpotyralski
Copy link

@LittleGreenYoda42 @dpotyralski Thanks, the rebase is done and the test build is now green. 🙌

Is there anything else missing to get this merged?

In general, it would probably be a good idea for someone else to manually test this change. Also note that I haven't tested any plugins and I'm not sure how isolated they are from the dependency changes here. I checked minor as the change type, but potentially this could be a breaking change and maybe it should be a major release?

Thanks @restfulhead for your input. I'm not sure if we are in a position to decide about the version here. Maybe @hipstersmoothie could add his opinion?

@restfulhead
Copy link
Contributor Author

Btw, if anyone would like to help test, I've released a beta version here: https://www.npmjs.com/package/@restfulhead/auto
But please don't depend on it. I might remove it at any time.

@Niceplace
Copy link

We still experience rate limiting errors here and there that prevent our workflows from properly executing, having up to date rate limiting/throttling plugins for Octokit would fix that. What can we do to help this PR get merged ?

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.

Github abuse rate limit error
5 participants