auto icon indicating copy to clipboard operation
auto copied to clipboard

chore: upgrade octokit dependencies

Open restfulhead opened this issue 1 year ago • 9 comments

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 https://github.com/intuit/auto/issues/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
  • [x] minor
  • [ ] major

restfulhead avatar May 02 '24 02:05 restfulhead

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

restfulhead avatar May 02 '24 16:05 restfulhead

Would love to see this get in!

hipstersmoothie avatar Jun 19 '24 19:06 hipstersmoothie

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

dpotyralski avatar Jul 01 '24 09:07 dpotyralski

@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.

LittleGreenYoda42 avatar Jul 01 '24 09:07 LittleGreenYoda42

@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?

restfulhead avatar Jul 03 '24 15:07 restfulhead

@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?

dpotyralski avatar Jul 04 '24 12:07 dpotyralski

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.

restfulhead avatar Jul 29 '24 17:07 restfulhead

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 ?

Niceplace avatar Oct 10 '24 13:10 Niceplace