WIP: Github Enterprise URL Addition

[TheFern2]

This is an enhancement as per #75, it adds an input text box for GHE Url that goes to the store settings. If there is a URL in this text box, then octokit uses the custom URL given for authentication.

@flashadvocate please download/clone this commit https://github.com/kodaman2/code-notes/tree/ghe-token:

• npm install
• npm run dev

I am having hard time finding documentation on the correct url for ghe please try the below, I assume it should be secured http requests:

• http://github.ncsu.edu
• https://github.ncsu.edu
• http://github.ncsu.edu/api/v3
• https://github.ncsu.edu/api/v3

Then add your personal token, and let me know how it goes, and which if any url worked.

Side note: I had to downgrade webpack-dev-server as per #92 in order to npm run dev

[lauthieb]

Hi! FYI, the commit from @jack-chapman is now merged on master branch :)

[TheFern2]

Hi! FYI, the commit from @jack-chapman is now merged on master branch :)

Latest master has been merged, and still awaiting a courageous enterprise user to test the PR lol.

[lauthieb]

I don't use Github Enterprise sorry :(

[flashadvocate]

Sorry sorry, I'll give it a go. Moving fast I see :)

[flashadvocate]

@kodaman2 getting bad credentials

Uncaught (in promise) HttpError: {"message":"Bad credentials","documentation_url":"https://developer.github.com/v3"}
    at IncomingMessage.response.on (/Users/dcdeaton/Projects/code-notes/node_modules/@octokit/rest/lib/request/request.js:73:18)
    at emitNone (events.js:111:20)
    at IncomingMessage.emit (events.js:208:7)
    at endReadableNT (_stream_readable.js:1056:12)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)

I am able to authenticate to the GHE server with my token though
curl -u username:<PAT> https://github.ncsu.edu/api/v3/gists

[
  {
    "url": "https://github.ncsu.edu/api/v3/gists/5ddd3d58a5e681de69118ddee425e05e",
    "forks_url": "https://github.ncsu.edu/api/v3/gists/5ddd3d58a5e681de69118ddee425e05e/forks",
    "commits_url": "https://github.ncsu.edu/api/v3/gists/5ddd3d58a5e681de69118ddee425e05e/commits",
    "id": "5ddd3d58a5e681de69118ddee425e05e",
...

[flashadvocate]

Not sure what's going on here... thought i might have found the issue, where the username was missing in the octokit authentication call, but so far no dice.

I've tried...

octokit.authenticate({
  type: 'basic',
  username: '[email protected]',
  password: '<my personal access token>'
});

octokit.authenticate({
  type: 'basic',
  username: 'myusername',
  password: '<my personal access token>'
});

octokit.authenticate({
  type: 'token',
  username: 'username',
  password: '<my personal access token>'
});

Nada

[TheFern2]

Ok I think I got to the bottom of this, hopefully lol. As it looks octokit/rest documentation site is only for version 16+ and you can't filter for older versions hence why this is not working, there has been tons of changes some which include parameter names and function names. I downloaded octokit/rest 14.0.8 the version code-notes is using and looked at the source code, and got a bit lucky that version had an example for github enterprise, thought I am not sure if only oauth auth works or token so I wrote two js scripts to see which one works or if both do.

https://github.com/kodaman2/octokit-api-test

Paste your token in the script before running it.

npm install
node index.js // this will test token auth
node enterprise.js // this will test oauth

Also just for added compatibility even if the above scripts work, I think I might suggest upgrading octokit/rest. So please test latest octokit version

Paste your token in the script before running it.

npm i @octokit/rest@latest // should install 16.32.0 please verify package.json
node latest.js

If neither of those yield any results a good idea might be to do vscode live share, and do a little remote debugging.

[TheFern2]

As a side note this is how much the interface changed from 14 to 16, that's why baseUrl wasn't working 😃, if latest.js works, I'll submit another PR to upgrade octokit version:

v14.0.8:

export interface Options {
    timeout?: number;
    host?: string;
    pathPrefix?: string;
    protocol?: string;
    port?: number;
    proxy?: string;
    ca?: string;
    headers?: {[header: string]: any};
    requestMedia?: string;
    rejectUnauthorized?: boolean;
    family?: number;
  }

v16.32.0:

export interface Options {
    auth?:
      | string
      | { username: string; password: string; on2fa: () => Promise<string> }
      | { clientId: string; clientSecret: string }
      | { (): string | Promise<string> };
    userAgent?: string;
    previews?: string[];
    baseUrl?: string;
    log?: {
      debug?: (message: string, info?: object) => void;
      info?: (message: string, info?: object) => void;
      warn?: (message: string, info?: object) => void;
      error?: (message: string, info?: object) => void;
    };
    request?: {
      agent?: http.Agent;
      timeout?: number;
    };
    timeout?: number; // Deprecated
    headers?: { [header: string]: any }; // Deprecated
    agent?: http.Agent; // Deprecated
    [option: string]: any;
  }

@lauthieb octokit/rest version 14.0.8 is nearly 2 years old with quite a few deprecated functions, given that it is only used for gists, do you want to stick with 14.0.8 or upgrade to the latest?

[flashadvocate]

@kodaman2 latest worked. The other two did not

[TheFern2]

@kodaman2 latest worked. The other two did not

ok, awesome, I'll work on upgrading octokit/rest then, and re-editing the request on this repo for ghe. That's good progress!

[TheFern2]

@flashadvocate could you retry index.js and enterprise.js, I've updated the url without the prefix parameter.

revert octokit/rest for those two.

npm i @octokit/[email protected]

[flashadvocate]

Results running both index.js and enterprise.js

[before-after-hook]: "Hook()" repurposing warning, use "Hook.Collection()". Read more: https://git.io/upgrade-before-after-hook-to-1.4
(node:20898) UnhandledPromiseRejectionWarning: Error: getaddrinfo ENOTFOUND https https:443
    at ClientRequest.request.on (/Users/dcdeaton/Projects/octokit-api-test/node_modules/@octokit/rest/lib/request/request.js:93:14)
    at ClientRequest.emit (events.js:198:13)
    at TLSSocket.socketErrorListener (_http_client.js:392:9)
    at TLSSocket.emit (events.js:198:13)
    at emitErrorNT (internal/streams/destroy.js:91:8)
    at emitErrorAndCloseNT (internal/streams/destroy.js:59:3)
    at process._tickCallback (internal/process/next_tick.js:63:19)

[TheFern2]

ok, well nvm I'll go ahead and update the package, is probably for the better anyways. Thanks for the results, I'll let you know when I integrate it on the app.

[flashadvocate]

No problem, thanks for the work. VueJS and electron seem to be a nice combination :)

[TheFern2]

No worries, I actually use gists quite a lot so I thought it was just to contribute to this repo.

[TheFern2]

Latest commit:

• upgraded octokit/rest from 14.0.8 to 16.32.0 as 14.0.8 was not working at all with ghe auth
• minor changes on octokit.gists calls, most functions stayed the same except edit was changed to update, and parameter id was changed to gist_id
• ran lint checks
• tested gists create, edit, delete local and checked remote gists
• merged with base branch

@flashadvocate tell me something good : ) I am hoping we are close to completion on this PR
Please fetch/clone latest commit on this branch:

npm install // ensure octokit/rest is upgraded to 16.32.0 on package.json
npm run dev

[flashadvocate]

@kodaman2 congrats. You've got a working implementation there ;)

[TheFern2]

@flashadvocate awesome! thanks for the update.

@lauthieb I believe this is ready for review, and merge. If you want to double check and make sure I didn't miss any octokit.gists calls but I believe I covered all my basis. : )

[lauthieb]

BIG THANKS @kodaman2 for your work on this PR :O
Thanks a lot @flashadvocate for your help on testing it.
Just a few changes please. Feel free to challenge my suggestions.

[TheFern2]

@lauthieb code changes requested have been fixed, was able to resolved all. Let me know what you think on the Octokit constructor fix, and I am open to suggestions I haven't been doing javascript for too long but code looks a lot cleaner and maintainable. Good tip on pulling version from package.json.

@flashadvocate functionality should work the same but please test to make sure, 8bf62f8

[TheFern2]

Ok, hopefully I understood the last change request.

[flashadvocate]

Seems to be working nicely as of the latest commit to this PR (tested up to fa256e4)

[TheFern2]

Seems to be working nicely as of the latest commit to this PR (tested up to fa256e4)

Cool thanks! it never hurts to check, would hate to merge it and then not work lol. A few more reviews and we should be good for merging. ; )

[lauthieb]

Hi @kodaman2, it's almost done :D
Can you please review my Pull Request on your branch here: TheFern2#1

@flashadvocate can you test this Pull Request also please? This is my branch : ghe-token-refactoring

[TheFern2]

Hi @kodaman2, it's almost done :D
Can you please review my Pull Request on your branch here: kodaman2#1

@flashadvocate can you test this Pull Request also please? This is my branch : ghe-token-refactoring

@lauthieb Merged, @flashadvocate you can test on my branch bd8b2da

[TheFern2]

@lauthieb how exactly do these changes work? I am just wondering, to be honest javascript constructors are super weird to my python/java eyes lol. Will need to read upon ES6 a bit more.

const getOctokit = ({ githubPersonalAccessToken, githubEnterpriseUrl }) => new Octokit({
  auth: githubPersonalAccessToken,
  userAgent: 'code-notes'.concat(packageJson.version),
  mediaType: {
    format: 'application/vnd.github.v3+json',
  },
  ...(githubEnterpriseUrl && { baseUrl: githubEnterpriseUrl }),
});

const octokit = getOctokit(store.rootState.Settings.settings);

[lauthieb]

@lauthieb how exactly do these changes work? I am just wondering, to be honest javascript constructors are super weird to my python/java eyes lol. Will need to read upon ES6 a bit more.

const getOctokit = ({ githubPersonalAccessToken, githubEnterpriseUrl }) => new Octokit({
  auth: githubPersonalAccessToken,
  userAgent: 'code-notes'.concat(packageJson.version),
  mediaType: {
    format: 'application/vnd.github.v3+json',
  },
  ...(githubEnterpriseUrl && { baseUrl: githubEnterpriseUrl }),
});

const octokit = getOctokit(store.rootState.Settings.settings);

First, you have a destructuring for settings when I get githubPersonalAccessToken and githubEnterpriseUrl.
Then we use the new keyword as Octokit documentation said.
Finally, I use a spread syntax and && logical AND, in this way, this line replace the if statement. If githubEnterpriseUrl is defined, then we can set a new key in this object named baseUrl spreaded in the object.
I found this tip there : https://stackoverflow.com/a/40560953

[TheFern2]

Thanks for the explanation. Makes sense, looks pretty lean too!

[flashadvocate]

Working great as of bd8b2da

[lauthieb]

Merged!
I will publish a new version as soon as possible :)

[lauthieb]

Thanks guys for this work.

[flashadvocate]

Merged!
I will publish a new version as soon as possible :)

Any chance for a release?

[lauthieb]

Hi @flashadvocate, I will do that on Saturday.

[lauthieb]

Here is the new release 1.2.3 of Code Notes : https://github.com/lauthieb/code-notes/releases/tag/1.2.3 🎉
Congrats @flashadvocate & @kodaman2!