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

[ENG-1747] Upgrade GitHub3.py library to 1.3.0 #9366

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

corbinSanders
Copy link
Contributor

Purpose

We are currently on 1.0.0a, which was released in 2016. We really should upgrade.

Changes

Modified addons/github/requirements.txt to include most recent version (1.3.0). Added user scope to settings. Modified a bunch of tests to include a mocked session, which is now required for the from_json method.

QA Notes

  • Does this change require a data migration? If so, what data will we migrate?
    No
  • What is the level of risk?
    High, please test all GitHub related functionality.
    • Any permissions code touched?
      No
    • Is this an additive or subtractive change, other?
      Modification
  • How can QA verify? (Through UI, API, AdminApp or AdminAdminApp?)
    UI
    • If verifying through API, what's the new version? Please include the endpoints in PR notes or Dev docs.
      N/A
  • What features or workflows might this change impact?
    GitHub addon
  • How will this impact performance?
    Shouldn't
    -->

Documentation

N/A

Side Effects

Shouldn't be.

Ticket

https://openscience.atlassian.net/browse/ENG-1747



branch_1 = Branch.from_json(dumps({
u'commit': {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we could phase out the u-strings for py3 that would be great. We should have a syntax moot some day and decide these details. The deal with u-strings in py3

Copy link
Contributor

@Johnetordoff Johnetordoff left a comment

Choose a reason for hiding this comment

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

One minor thing, but looks very good. Excellent mocking!

@@ -1,5 +1,8 @@
import mock
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait shouldn't this be addons/github/tests/utils.py, not addons/gitlab/tests/utils.py?

@@ -52,7 +52,7 @@ def get_path(kwargs, required=True):
raise HTTPError(http_status.HTTP_400_BAD_REQUEST)


def get_refs(addon, branch=None, sha=None, connection=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow all those test changes for this one function...

@Johnetordoff
Copy link
Contributor

@corbinSanders oops! one more thing can you see if you can remove these lines there's a few more instances of uritemplate bug fixing too.

@Johnetordoff
Copy link
Contributor

Johnetordoff commented May 1, 2020

@corbinSanders What about these lines ?

@corbinSanders
Copy link
Contributor Author

@corbinSanders What about these lines ?

👌


def create_mocked_session():
"""Use mock to auto-spec a GitHubSession and return an instance."""
MockedSession = mock.create_autospec(github3.session.GitHubSession)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, that seems fancy.

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.

None yet

3 participants