-
Notifications
You must be signed in to change notification settings - Fork 31
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
Freshen up deps #93
Freshen up deps #93
Conversation
Ok, deps are all up to date. This first and foremost will remove the security warnings. Unfortunately, ghauth doesn't have a fix for the new endpoint yet, but I opened an upstream issue: rvagg/ghauth#26 I would hold off from releasing this (we can land the PR though) until I figure out a solution for the auth flow. It would be a nice contribution to ghuath to update it to the latest auth endpoint. |
If this isn't a breaking change why not release now? |
A bunch of deps raised the minimum node version required to run, so from that perspective its breaking, but if we already assumed LTS was always the minimum it wouldn't be. wdyt? Breaking would be safer but more (easy) downstream work. |
The new auth flow would likely be breaking, so I figured I would roll all this in with that. |
ok makes sense to me. just like to avoid leaving things unreleased in master when possible -- we're all busy and it's entirely possible the new auth flow won't get dealt with soon |
Sounds good. If the auth stuff stalls out for a bit, I can cut a release with just this. Whats your preference semverwise on just this portion of the work? (I think it should be major to be on the safe side). |
considering this gets 16k downloads / month it would probably be better to be cautious and cut a major release -- I've been bitten enough by other module authors releasing breaking changes under a patch or minor to try to never do that to anyone else. we could wait to merge this until the auth flow stuff is also ready to not do two major releases |
Ok sounds good. Ill make a call to merge over the next week or so depending on upstream interest in the new device auth flow. |
thanks bret, sorry to complicate it. appreciate the work |
Doing the good lordt's work here, @bcomnes much appreciated 👍 |
Pr opened: rvagg/ghauth#27 Any reviews appreciated. Biggest change that affects gh-release is the change from
The reason being, there are a ton more URLs and subdomains involved now, so providing the host seems like the best thing to customize. UPDATE: This is no longer true. |
Looks like we've stalled out upstream. Thinking now that we should land this work, and then just break again when upstream gh auth is out. We have till November in theory. |
@bcomnes we could fork ghauth until upstream is responsive -- not sure if rvagg is actively maintaining ghauth anymore |
Definitely an option I was thinking about. Would be nice to at least get his input on the desired outcome. I'll give it a couple of weeks. |
Ok, I forked ghauth for the time being, while we wait for upstream. Going to test this out a few more times and cut a major release. |
Unaffected by the breaking changes other than dropped node versions.
How many more majors will yargs get! Nobody knows! At least it was easy.
Doesn't fix our deprecated endpoint issue, but it does update to the latest version of the dep. I opened an upstream issue regarding the endpoint: rvagg/ghauth#26
Co-authored-by: john gravois <jagravois@gmail.com>
Also not to stop using basic auth unless you are using GHE.
7d6457b
to
20c2121
Compare
Found a bug where the error messaging is terrible when you try to release to an org that is lacking permissions for the new oauth scoped tokens. Going to improve that. |
OK, going to cut a major on this. |
Ok this is out in |
Thanks @bcomnes ! |
Looking to close #92. Starting with all the outdated deps to get those out of the way.
The deps that need code changes appear to be:
Working on that part next.