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

feat: update octokit #360

Merged
merged 25 commits into from
Jan 14, 2021
Merged

feat: update octokit #360

merged 25 commits into from
Jan 14, 2021

Conversation

jugaltheshah
Copy link
Contributor

@jugaltheshah jugaltheshah commented Jan 9, 2021

Deprecated - Github username & password auth:
As Github has deprecated REST API auth via username & password, this no longer works as an auth mechanism. After this PR, shepherd will still support auth via .netrc, but the .netrc.password field should supply a token instead of a Github password.

Updates:

  • "@octokit/rest": "^15.18.1" -> "@octokit/rest": "^18.0.12",,
    • octokit methods, e.g. octokit.pullRequests.update -> octokit.pulls.update

@jugaltheshah jugaltheshah changed the title feat: update octokit [WIP] feat: update octokit Jan 9, 2021
@jugaltheshah jugaltheshah marked this pull request as draft January 10, 2021 01:06
@jugaltheshah jugaltheshah changed the title [WIP] feat: update octokit feat: update octokit Jan 10, 2021
src/adapters/github.ts Outdated Show resolved Hide resolved
@jugaltheshah jugaltheshah marked this pull request as ready for review January 10, 2021 09:39
@aorinevo
Copy link
Collaborator

aorinevo commented Jan 10, 2021

@nwalters512, I recommend bumping the major version here and providing support for the last 2 major versions. Thoughts?

@aorinevo
Copy link
Collaborator

@jugaltheshah, as this is a breaking change, let's bump the major version.

src/adapters/github.ts Outdated Show resolved Hide resolved
@jugaltheshah
Copy link
Contributor Author

@nwalters512, I recommend bumping the major version here and providing support for the last 2 major versions. Thoughts?

That was the intent with the "breaking change" in the PR description though come to think of it, I'm not sure if that's a custom option that this repo doesn't have enabled. Regardless, I'll update the PR title, which should do the same.

Please be sure to squash & merge this PR so semantic bot will cut a new major release

@jugaltheshah jugaltheshah changed the title feat: update octokit feat: (BREAKING CHANGE) update octokit Jan 10, 2021
@aorinevo
Copy link
Collaborator

@nwalters512, I recommend bumping the major version here and providing support for the last 2 major versions. Thoughts?

That was the intent with the "breaking change" in the PR description though come to think of it, I'm not sure if that's a custom option that this repo doesn't have enabled. Regardless, I'll update the PR title, which should do the same.

Please be sure to squash & merge this PR so semantic bot will cut a new major release

Need to confirm that this is the expected behavior.

@aorinevo
Copy link
Collaborator

@jugaltheshah, mind squashing the commits down prior to merging this down. In addition, can you follow the message commit format is specified in [1].

References

  1. https://www.conventionalcommits.org/en/v1.0.0/

@nwalters512
Copy link
Collaborator

I recommend bumping the major version here and providing support for the last 2 major versions. Thoughts?

Is the breaking change the difference in how .netrc works? I'm on board with calling this v2, yeah.

By "providing support for the last 2 major versions", do you mean backporting future changes to v1? I don't think that'll be necessary.

@jugaltheshah
Copy link
Contributor Author

I recommend bumping the major version here and providing support for the last 2 major versions. Thoughts?

Is the breaking change the difference in how .netrc works? I'm on board with calling this v2, yeah.

By "providing support for the last 2 major versions", do you mean backporting future changes to v1? I don't think that'll be necessary.

That's an interesting question actually. I'm actually now questioning if we actually need to mark this as breaking.

Yes, the reason this PR is currently breaking is the difference in how .netrc works. However, the docs say you can use a token in place of a password for clients that only support basic auth. So I'm wondering, can we can keep the existing .netrc un/pw flow, but assume the password supplied is actually a token?

So then if we keep .netrc and use the supplied username & password (token), would this still be breaking (keeping in mind that using password as password hass been broken)?

cc @aorinevo

@aorinevo
Copy link
Collaborator

Netrc should still work, right? It would potentially start to degrade during the brownout period and then be fully non-functional when username/password is deprecated.

I'm still in favor of doing a major version bump.

@aorinevo
Copy link
Collaborator

I recommend bumping the major version here and providing support for the last 2 major versions. Thoughts?

Is the breaking change the difference in how .netrc works? I'm on board with calling this v2, yeah.

By "providing support for the last 2 major versions", do you mean backporting future changes to v1? I don't think that'll be necessary.

Security and bug fixes only for the last major version - no backporting of features/enhancements.

@jugaltheshah
Copy link
Contributor Author

Netrc should still work, right? It would potentially start to degrade during the brownout period and then be fully non-functional when username/password is deprecated.

I'm still in favor of doing a major version bump.

Using username & password against the Github REST API was already deprecated as of November 13th, 2020 per this doc.

I think what's causing some confusion here is that Github deprecated using username:password auth, but not basic auth via username:token with token used like a password. So if your Github username & password were user1:password123, using those against the Github API won't work. However, if you generate a personal access token on Gihub.com and use user1:token123 against the Github API, I believe this will still work (this is what I still need to confirm). They did this for compatibility with clients that only support basic auth. Of course we don't actually need a username since tokens are associated with users already, so really we'd just need to assume a password provided is a token.

So we have a couple options:

  1. Keep using .netrc but re-purpose the login field to be used as a token --> breaking, but I believe this is what octokit/netrc does. This is what this PR currently does also.
  2. Keep using .netrc, keep reading from password, but assume password provided = Github token, not Github password --> not necessarily breaking*
  • Not necessarily breaking from our perspective because as previously noted, this would have already been broken as of mid-Nov of last year. If users currently have their Github password in .netrc, it shouldn't work, but if they update it to a personal access token and we go with option 2, it should work.

We could err on the side of caution here and release a major version regardless, but just wanted to present the options as I saw them.

@aorinevo
Copy link
Collaborator

Ah, I see now. That makes sense.

I'm in favor of option 2 and keeping this a minor bump.

@jugaltheshah jugaltheshah changed the title feat: (BREAKING CHANGE) update octokit feat: update octokit Jan 11, 2021
@aorinevo
Copy link
Collaborator

Many thanks for this contribution @jugaltheshah! 🚢

@aorinevo aorinevo merged commit 5604e4f into NerdWalletOSS:master Jan 14, 2021
aorinevo pushed a commit that referenced this pull request Jan 14, 2021
# [1.13.0](v1.12.0...v1.13.0) (2021-01-14)

### Features

* update octokit ([#360](#360)) ([5604e4f](5604e4f))
@aorinevo
Copy link
Collaborator

🎉 This PR is included in version 1.13.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

@jugaltheshah jugaltheshah deleted the octokit-18 branch January 14, 2021 00:25
jugaltheshah added a commit to jugaltheshah/shepherd that referenced this pull request Jan 21, 2021
jugaltheshah pushed a commit to jugaltheshah/shepherd that referenced this pull request Jan 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants