Fix GitHub basic token auth, upgrade GitHub API module, allow test suite to execute. #319
Conversation
Add v3 endpoints (and GitLab support)
The function call which was removed appears to have no reference in the codebase.
alexwaibel
added
needs docs
… basic token auth.
Get the remainder of the GitHub libarary unit tests in passing state. Should rewrite more of the tests to use Nock rather than mocking the module with Jest at some point in the future. Bump Nock to latest version. Use correct key import per RSA documentation Enable logging. Use V2 requests for most of tests
|
Ok, I got the unit test failures down from ~70 to a more manageable 20. I switched some of the GitHub tests to nock, though I think it would be good to switch all of those tests specifically to nock in the future as a backlog item. I know that makes it less of a unit test in the strict sense, but I think it's more useful to test that we're hitting the right github endpoints rather than that we're hitting the right api module methods.. |
|
Alright, GitHub library is completely rewritten to use Nock. While I was at it, I converted the whole thing to ES6, with the exception of the imports which Jest didn't like as ES6. |
I believe V3 was meant to support both methods, but yes; that's the general idea. Changes look great, thank you! Feel free to merge. 👏 💪 |
|
Thanks for the review Eduardo. I'm going to investigate the app auth a bit more before merging this. Once I've finished that, I'd like to merge this into dev and then merge dev into master with some documentation updates. |
|
Sounds great! |
|
I'm so happy to see this repo/project getting some much needed love again. Thanks guys! |
Instructions for creating a private instance hosted on the free tier of Heroku have been added and all references to the public instance have been removed. Heroku hosting is intended to provide users with a more reliable alternative to the public instance, which we will be dropping support for. Many changes had to be made to enable authentication as a GitHub application. This required a lot of methods to become async.
|
Alright @eduardoboucas I think I have this in a state I would be comfortable merging into dev and master after another review since I made a lot of changes with my most recent commit. I've fixed GitHub app integration, which required a lot of async conversion. I've updated the README to include instructions on Heroku deployment and removed all references to the public instance. I think it's best to stop recommending the public instance, but continue to leave it running as-is to avoid breaking anyone's implementations. Self-hosting on Heroku is free and very easy so this should provide an appropriate alternative. |
alexwaibel
removed
in development
|
The v3 endpoint for connect is missing because of this fix. Without this, it doesn't connect to GitLab. Can this be restored, please? See changes made to server.js: https://github.com/eduardoboucas/staticman/pull/219/files#diff-78c12f5adc1848d13b1c6f07055d996e Please LMK if I'm missing something. New to the project. Thanks for all your work! |
@jayliu50 I've been a bit swamped with other projects lately, but I'd happily accept a PR for a fix. I don't have time right now to dig too deep into this. I never got around to trying this out with GitLab, but it seems to me that GitLab never used the connect endpoint. If you look at the PR you linked, in I believe @VincentTam worked with the GitLab parts of this project a lot. Vincent, do you have any insight here? |
|
My work mainly consists in testing the linked PR. Yeah GitLab doesn't use the
|
It seems that the Lines 48 to 54 in 5d7ed77 |
This is a step towards fixing #318 and though I haven't fixed the related test cases I figured it would be good to get this up here so others can start reviewing it.
I've bumped dev to the latest version of master. Most of these changes lie in Github.js and aim to simplify the authentication flow. I've preserved the version information which some other PRs have reverted.
From your previous commits @eduardoboucas , I believe the versioning intention to be:
V2 API: To be used with Github personal token authentication only
V3 API: To support Github bot authentication only
Can you confirm this?
Additionally, I've fixed the style warnings that were stopping the unit tests from running. I haven't yet fixed the unit tests for the GitHub library but I plan to before we merge this into master.
For a demonstration you can visit my test site, which should be working unless I change it to test something else.
That site is running this config and you can view the PRs it makes in that same repo.