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

@actions/github v3 using Octokit/core #453

Merged
merged 11 commits into from
Jun 3, 2020
Merged

Conversation

thboop
Copy link
Collaborator

@thboop thboop commented May 13, 2020

@actions/github is stuck on an older major version of @octokit/rest due to the way we extended the class. Let's move away from extending the Octokit class so we don't encounter this again, and move towards the plugin/defaults model all the JS libraries have been using.

This allows us to customize the octokit is a variety of ways, and reuse existing plugins easily.
In particular, we currently were using @octokit/rest which is just @octokit/core with 3 plugins layered on top:

  • @octokit/plugin-paginate-rest - provides the paginate function to paginate rest calls
  • @octokit/plugin-rest-endpoint-methods - provides the bulk of the rest apis accessible via functions
  • @octokit/plugin-request-log - provides additional logging for rest calls

We don't really need the latter, so we can use @octokit/core to set up a custom octokit with the paginate and rest endpoint plugins and set sane defaults using environmental variables to automatically handle proxy and GHES url scenarios.

We can also choose to implement our own custom plugins as needed going forward, or reuse some of the existing plugins, like GHES api plugins

Why don't we just keep using @octokit/rest?
The core library is really intended to be used if you are extending the plugin model. Type information does not carry well over multiple exports when using plugins, and this offers far more flexibility (at the cost of having to update more packages)

Resolves #468 , #402

@@ -0,0 +1,109 @@
import * as http from 'http'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We now load proxy settings when the module is imported because of the way defaults works. This required a new test suite for proxy tests.

packages/github/src/github.ts Outdated Show resolved Hide resolved
@ericsciple
Copy link
Contributor

Should we provide a factory method, to help users mock. Might buy us flexibility also (abstraction) instead of inheriting.

For example:

export function createOctokit(token: string, opts?: OctokitOptions): Octokit {
}

Would the types get exported correctly?

Would we need to overload to enable custom list of plugins?

packages/github/src/github.ts Outdated Show resolved Hide resolved
packages/github/src/github.ts Outdated Show resolved Hide resolved
@thboop thboop requested a review from jclem May 20, 2020 01:08
@thboop
Copy link
Collaborator Author

thboop commented May 20, 2020

Should we provide a factory method, to help users mock. Might buy us flexibility also (abstraction) instead of inheriting.

For example:

export function createOctokit(token: string, opts?: OctokitOptions): Octokit {
}

Would the types get exported correctly?

Would we need to overload to enable custom list of plugins?

I exported a helper function called getOptions(token: string, opts?: OctokitOptions) which supports this scenario nicely.However, it is a little verbose as you have to pass the output of that function into the constructor like

import {getOptions, GitHub} from `@actions/github`
import {getInput} from `@actions/core`
const token = getInput("token")

const octokit = new GitHub(getOptions(token))

// or pass in octokit options as well
const octokit = new GitHub(getOptions(token{ timeZone: "America/Los_Angeles"}))

// you may also choose to just format your octokit options correctly
const octokit = new github.GitHub({auth: `token ${myToken}`});

But getOptions also allows for people to expand on our Instance as needed.

import {getOptions, GitHub} from `@actions/github`
import {getInput} from `@actions/core`
import {myPlugin} from `my-custom-plugin`
const myGitHub = GitHub.plugin(myPlugin)

const token = getInput("token")
const octokit = new myGitHub(getOptions(token), {//whatever octokit options you want})
// use your new octokit with your plugins, or a community plugin

@thboop thboop marked this pull request as ready for review May 21, 2020 20:58
"@octokit/rest": "^16.43.1"
"@octokit/core": "^2.5.1",
"@octokit/plugin-paginate-rest": "^2.2.0",
"@octokit/plugin-rest-endpoint-methods": "^3.10.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious whether there is a big impact to overall size on disk?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There shouldn't be, @octokit/rest imported all 3 of these modules. i'll check but it should be less

@thboop thboop requested a review from ericsciple May 27, 2020 17:45
@thboop thboop merged commit 4a89cf7 into master Jun 3, 2020
@thboop thboop deleted the users/thboop/updateOctokit branch June 3, 2020 01:39
fhammerl pushed a commit to actions/checkout that referenced this pull request Apr 12, 2023
…/github dependency (#1246)

* Improve checkout performance on Windows runners by upgrading @actions/github dependency

Re: #1186

@dscho discovered that the checkout action could stall for a
considerable amount of time on Windows runners waiting for PowerShell
invocations made from 'windows-release' npm package to complete.

Then I studied the dependency chain to figure out where
'windows-release' was imported:

'@actions/checkout'@main
  <- '@actions/github'@2.2.0
    <- '@octokit/endpoint'@6.0.1
    <- '@octokit/graphql'@4.3.1
    <- '@octokit/request'@5.4.2
    <- '@octokit/rest'@16.43.1
      <- 'universal-user-agent'@4.0.1
        <- 'os-name'@3.1.0
          <- 'windows-release'@3.1.0

'universal-user-agent' package dropped its dependency on 'os-name' in
https://github.com/gr2m/universal-user-agent/releases/tag/v6.0.0 .

'@actions/github' v3 removed dependency on '@octokit/rest'@16.43.1 and
allows users to move away from the old 'universal-user-agent' v4.
(actions/toolkit#453)

This pull request attempts to update the version of '@actions/github'
used in the checkout action to avoid importing 'windows-release'.

Based on testing in my own repositories, I can see an improvement in
reduced wait time between entering the checkout action and git actually
starts to do useful work.

* Update .licenses

* Rebuild index.js
DymysClaflin3 pushed a commit to DymysClaflin3/checkout that referenced this pull request Apr 29, 2023
…/github dependency (#1246)

* Improve checkout performance on Windows runners by upgrading @actions/github dependency

Re: actions/checkout#1186

@dscho discovered that the checkout action could stall for a
considerable amount of time on Windows runners waiting for PowerShell
invocations made from 'windows-release' npm package to complete.

Then I studied the dependency chain to figure out where
'windows-release' was imported:

'@actions/checkout'@main
  <- '@actions/github'@2.2.0
    <- '@octokit/endpoint'@6.0.1
    <- '@octokit/graphql'@4.3.1
    <- '@octokit/request'@5.4.2
    <- '@octokit/rest'@16.43.1
      <- 'universal-user-agent'@4.0.1
        <- 'os-name'@3.1.0
          <- 'windows-release'@3.1.0

'universal-user-agent' package dropped its dependency on 'os-name' in
https://github.com/gr2m/universal-user-agent/releases/tag/v6.0.0 .

'@actions/github' v3 removed dependency on '@octokit/rest'@16.43.1 and
allows users to move away from the old 'universal-user-agent' v4.
(actions/toolkit#453)

This pull request attempts to update the version of '@actions/github'
used in the checkout action to avoid importing 'windows-release'.

Based on testing in my own repositories, I can see an improvement in
reduced wait time between entering the checkout action and git actually
starts to do useful work.

* Update .licenses

* Rebuild index.js
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.

Update @octokit dependencies
3 participants