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

fix(publisher-github): remove octokit deprecation warnings #1673

Closed
wants to merge 3 commits into from

Conversation

b-zurg
Copy link
Contributor

@b-zurg b-zurg commented May 7, 2020

  • I have read the contribution documentation for this project.
  • I agree to follow the code of conduct that this project follows, as appropriate.
  • The changes are appropriately documented (if applicable).
  • The changes have sufficient test coverage (if applicable).
  • The testsuite passes successfully on my local machine (if applicable).

Summarize your changes:

Implemented API Changes in order to remove deprecation warnings from github publisher.

Closes #1672

Copy link
Member

@malept malept left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the pull request! I take it you've tested these changes manually?

All that's needed at this point is to fix the tests, which are unfortunately kind of fragile due to the module needing to access the network normally.

https://github.com/electron-userland/electron-forge/blob/1cfe04989c0976a1317c4b91c6467f7986ddda68/packages/publisher/github/test/github_spec.ts#L68-L71

  • It looks like the following tests need to be rewritten to not use mocks:

https://github.com/electron-userland/electron-forge/blob/1cfe04989c0976a1317c4b91c6467f7986ddda68/packages/publisher/github/test/github_spec.ts#L74-L86

They'll probably look closer to the user agent tests.

@malept malept changed the title fix: github octokit deprecation warnings fixed fix(publisher-github): remove octokit deprecation warnings May 12, 2020
@mahnunchik
Copy link
Contributor

@b-zurg could you please update the tests to have changes merged?

@b-zurg
Copy link
Contributor Author

b-zurg commented Jun 11, 2020

I'll take care of it tomorrow. Sorry it's taken me so long to finish this up.

@b-zurg
Copy link
Contributor Author

b-zurg commented Jun 12, 2020

I've made some of the necessary changes. I did a bit of investigation and it doesn't seem like an authenticate is necessary anymore for the GitHub class as all it did was set this's authentication token to the one passed in. So it didn't seem like the tests were necessary anymore.

I've had a lot of trouble getting bolt and tests running well, the dev cycle is very slow because of the extensive amount of tests, however I hope to get them running soon on my local env.

This PR is not ready for merge yet.

@malept malept marked this pull request as draft June 12, 2020 20:50
@malept
Copy link
Member

malept commented Jun 12, 2020

For your purposes, you should be able to filter out most tests, by running:

yarn test --glob=test/github_spec.ts

It's something I need to add to CONTRIBUTING.md.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Github Publisher Using Deprecated Options
3 participants