octokit client should follow proxy settings #314
Conversation
| import * as http from 'http' | ||
| import {GitHub} from '../src/github' | ||
|
|
||
| describe('@actions/github', () => { |
There was a problem hiding this comment.
New tests to exercise GitHub REST client and GraphQL client with and without proxy
Only runs when env var GITHUB_TOKEN is defined, otherwise warns for discoverability
| @@ -1,5 +1,6 @@ | |||
| { | |||
| "compilerOptions": { | |||
| "esModuleInterop": true, | |||
There was a problem hiding this comment.
I moved this setting from packages/github/tsconfig.json to here.
It's needed for the GitHub unit tests to work. The unit tests only follow the root tsconfig.json.
Without this setting, the base class properties on GitHub don't work. For example, attempting to access the property repos fails:
const octokit = new GitHub(token)
const branch = await octokit.repos.getBranch({
owner: 'actions',
repo: 'toolkit',
branch: 'master'
})Also Typescript "highly recommend applying it" so this seems OK to move up to the root tsconfig.json
There was a problem hiding this comment.
@hross thoughts regarding consumer impact. I'm assuming shouldnt be any
| @@ -5,7 +5,7 @@ import * as os from 'os' | |||
| import * as path from 'path' | |||
| import * as httpm from '@actions/http-client' | |||
| import * as semver from 'semver' | |||
| import * as uuidV4 from 'uuid/v4' | |||
| import uuidV4 from 'uuid/v4' | |||
There was a problem hiding this comment.
reaction due to moving "esModuleInterop": true to the root tsconfig.json - which previously was only defined in packages/github/tsconfig.json
| import * as path from 'path' | ||
| import * as io from '@actions/io' | ||
| import * as exec from '@actions/exec' | ||
| import nock from 'nock' |
There was a problem hiding this comment.
reaction due to moving "esModuleInterop": true to the root tsconfig.json - which previously was only defined in packages/github/tsconfig.json
| @@ -48,8 +48,13 @@ export class Pattern { | |||
| // https://github.com/typescript-eslint/typescript-eslint/issues/291 | |||
|
|
|||
| constructor(pattern: string) | |||
| constructor(pattern: string, segments: undefined, homedir: string) | |||
There was a problem hiding this comment.
all the changes to this file are reaction due to moving "esModuleInterop": true to the root tsconfig.json - which previously was only defined in packages/github/tsconfig.json
mocking os in the module registry from the unit tests, stopped working when esModuleInterop=true so i switched to a constructor overload pattern
| import * as path from 'path' | ||
| import * as pathHelper from './internal-path-helper' | ||
| import assert from 'assert' |
There was a problem hiding this comment.
reaction due to moving "esModuleInterop": true to the root tsconfig.json - which previously was only defined in packages/github/tsconfig.json
| import * as path from 'path' | ||
| import assert from 'assert' |
There was a problem hiding this comment.
reaction due to moving "esModuleInterop": true to the root tsconfig.json - which previously was only defined in packages/github/tsconfig.json
| @@ -1,17 +1,9 @@ | |||
| import * as io from '../../io/src/io' | |||
| import * as os from 'os' | |||
There was a problem hiding this comment.
all the changes to this file are reaction due to moving "esModuleInterop": true to the root tsconfig.json - which previously was only defined in packages/github/tsconfig.json
i left a comment on internal-pattern.ts (further below) which explains more
| @@ -2,7 +2,6 @@ | |||
| "extends": "../../tsconfig.json", | |||
| "compilerOptions": { | |||
| "baseUrl": "./", | |||
| "esModuleInterop": true, | |||
There was a problem hiding this comment.
moved up to the root tsconfig.json
i left a comment on the root tsconfig.json (further below) which explains this more
| this.graphql = graphql.defaults({ | ||
| headers: {authorization: `token ${token}`} | ||
| }) | ||
| /** |
There was a problem hiding this comment.
I made a change to allow users to specify their own auth settings (in opts) instead of providing a token.
Customers who are using other authentication methods should still be able to use this class for the proxy benefits.
The one quirky detail is graphql auth only supports a string. So if token is provided or opts.auth is a string, then i setup the graphql authorization header. But if opts.auth is an object, then i dont set the graphql authorization header
| @@ -0,0 +1,5 @@ | |||
| declare module 'proxy' { | |||
There was a problem hiding this comment.
typescript made me do this :(
|
👀 |
As actions/tool-cache 1.1.2 is not following proxy settings e.g. `http_proxy`, `https_proxy`, ... - we need to update to version 1.3.1 where that is solved: actions/toolkit#314 Signed-off-by: Jasmin Beharic (jabe13) <jasmin.z.beharic@teliacompany.com>
As actions/tool-cache 1.1.1 is not following proxy settings e.g. `http_proxy`, `https_proxy`, ... - we need to update to version 1.3.1 where that is solved: actions/toolkit#314 Signed-off-by: Jasmin Beharic (jabe13) <jasmin.z.beharic@teliacompany.com>
No description provided.