Change passport-github to passport-github2 and @octokit/rest to v18 by addison-grant · Pull Request #211 · iFixit/pulldasher

addison-grant · 2021-05-05T16:07:11Z

This library seems popular and claims to use a different auth method
that I think will bandaid our current brownout issue.

New package: https://github.com/cfsghost/passport-github
Deprecation notice: (Links to original notice)
https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/

Changes made:

Removed the usage of the npm package passport-github in favor of passport-github2
Updated the npm package @octokit/rest to v18
By updating @octokit/rest , the function calls in lib/git-manager.js had to be updated to reflect the new function calls that call the same endpoints (Change passport-github to passport-github2 and @octokit/rest to v18 #211 (comment)). Also had to update the checkOrgMembershipRequirements function in lib/authentication.js
Removed the use of the rate-limit function as the plugin @octokit/plugin-throttling now allows for the GitHub API object to throttle itself (https://github.com/octokit/plugin-throttling.js#readme)
Removed the use of the paginate function as @octokit/rest v18 now has its own pagination function.

QA

You can run update-pulldasher on cominor with a clone of this repo with this branch checked out, and check out https://pulldasher.cominor.com. Look around pulldasher, let it run throughout the day and make sure the cards reflect the state of their PRs at the end of the day.

Associated issue: #210

This library seems popular and claims to use a different auth method that I think will bandaid our current brownout issue. New package: https://github.com/cfsghost/passport-github Deprecation notice: (Links to original notice) https://developer.github.com/changes/2020-02-10-deprecating-auth-through-query-param/

addison-grant · 2021-05-05T16:08:03Z

Need to test this out. I checked the library and it seemed legit, as in didn't notice anything that seemed bad...can a couple other people double-check?

addison-grant · 2021-05-05T16:18:50Z

Wasn't a silver bullet, sadly.

addison-grant · 2021-05-05T17:04:00Z

dev_block ☀️ I think this might be needed, but there's more changes needed, as well.

ardelato · 2021-05-05T23:24:47Z

Test Comment -- Webhook

addison-grant

test

addison-grant · 2021-05-05T23:51:16Z

The test comments were to test out comment webhooks.

addison-grant · 2021-05-05T23:52:04Z

Some relevant docs:

https://octokit.github.io/rest.js/v18

danielbeardsley · 2021-05-10T23:02:05Z

Thanks @addison-grant for working on this!!

andyg0808 · 2021-05-12T00:40:21Z

    "notify-queue": "0.0.5",
    "passport": "^0.4.0",
-    "passport-github": "1.1.0",
+    "passport-github2": "^0.1.12",


I looked through the diff between passport-github and passport-github2, and it looks totally reasonable. I'm not quite sure how hard we want to version lock this; I'm nervous about the fact that this is one semi-random guy's fork.

(when I say "how hard we want to version lock this", I mean "do we want to use a version like

Suggested change

"passport-github2": "^0.1.12",

"passport-github2": "0.1.12",

which won't allow any other version to be used" (although I don't think it prevents malicious uploads over 0.1.12 on NPM).

The package looks pretty small, actually, looking over at https://github.com/cfsghost/passport-github/tree/master/lib. I wonder if we would want to fork passport-github2, take a little time to understand it, and install from our fork.

andyg0808 · 2021-05-12T01:06:09Z

 function getCommit(repo, sha) {
-   return rateLimit(github.repos.getCommit)(params({ sha }, repo))
-                      .then(res => res.data);
+   return githubRest.repos.getCommit(params({ ref: sha }, repo)).then(res => res.data);


Is the res.data still needed?

I'm not sure...I'll look into this.

I think this is still needed because we need the data property off the response. Was there a particular reason you thought we don't need it?

andyg0808 · 2021-05-12T01:11:09Z

   debug("Getting commit status for %s", ref);
-   return rateLimit(github.repos.getCombinedStatusForRef)(params({ ref }, repo))
+   return githubRest.repos.getCombinedStatusForRef(params({ ref }, repo))
      .then(res => res.data.statuses)


Is the res.data still needed here as well?

same here, will investigate

I think this is still needed because we need the particular sub-property off the response. Was there a particular reason you thought we don't need it?

andyg0808 · 2021-05-13T19:02:26Z

CR 🐟 through 5dca5e7
deploy_block 👍 on comments.

Co-authored-by: Andrew Gilbert <andrewg@ifixit.com>

We want to keep the colors around because it's a great indication as to how old the pulls are at a glance. Let's match the colors to the preexisting colors that we use on the pull cards. QA: -- Make sure the colors look good

Bumps [url-parse](https://github.com/unshiftio/url-parse) from 1.4.7 to 1.5.1. - [Release notes](https://github.com/unshiftio/url-parse/releases) - [Commits](unshiftio/url-parse@1.4.7...1.5.1) Signed-off-by: dependabot[bot] <support@github.com>

Bumps [hosted-git-info](https://github.com/npm/hosted-git-info) from 2.8.5 to 2.8.9. - [Release notes](https://github.com/npm/hosted-git-info/releases) - [Changelog](https://github.com/npm/hosted-git-info/blob/v2.8.9/CHANGELOG.md) - [Commits](npm/hosted-git-info@v2.8.5...v2.8.9) Signed-off-by: dependabot[bot] <support@github.com>

Bumps [underscore](https://github.com/jashkenas/underscore) from 1.9.1 to 1.12.1. - [Release notes](https://github.com/jashkenas/underscore/releases) - [Commits](jashkenas/underscore@1.9.1...1.12.1) Signed-off-by: dependabot[bot] <support@github.com>

…ort-to-alternative-v2

TODO: update schema

deltuh-vee · 2021-07-16T18:04:12Z

    github = require('./git-manager').github,
    passport = require('passport'),
    _ = require('underscore'),
-    GitHubStrategy = require('passport-github').Strategy;


test dev_block 👍

hello -Addison

addison-grant · 2021-07-16T18:13:15Z

-   const getTeamId = github.orgs.getTeams({org: config.github.requireOrg})
+
+   // `github` here is octokit.rest imported from git-manager
+   const getTeamId = github.teams.list({org: config.github.requireOrg})


Trying to test a pull_request_review event

A single comment on a diff

danielbeardsley · 2021-08-02T20:55:17Z

I'm a little worried why there are commits in here from dependabot. I'm gonna look at the commit history.

danielbeardsley · 2021-08-02T20:57:57Z

Update: Those exact commit hashes are already on master and github's diff is just showing the wrong thing.

New github octokit has new api options and the team stuff no longer requires a teamid and can jut work with the team name.

New api call returns data for single-record requests under the data key. Other endpoints were updated, but this one was missed.

It was using the wrong api after updating to the new octokit.

danielbeardsley · 2021-08-07T00:35:20Z

Working on this. Still more to review.

I think this was introduced accidentally in a recent merge.

The prototype function references a ._then property that didn't exist on github promises, so let's just cast each one to a bluebird promise before calling done.

danielbeardsley · 2021-08-10T23:26:46Z

QA 👍

Verified this with @addison-grant ! It lives!

danielbeardsley · 2021-08-10T23:26:53Z

CR 👍

addison-grant closed this May 5, 2021

addison-grant reopened this May 5, 2021

addison-grant commented May 5, 2021

View reviewed changes

Comment thread lib/authentication.js

Updating with new octokit endpoints

d4f3e97

addison-grant mentioned this pull request May 5, 2021

Pulldasher authenticated call method is deprecated. #210

Closed

addison-grant changed the title ~~Use alternative library for passport-github~~ Change passport-github to passport-github2 and @octokit/rest to v18 May 5, 2021

Modified the last portions where we used the old endpoints

5dca5e7