8000 breaks with older github enterprise versions · Issue #794 · sigmavirus24/github3.py · GitHub
[go: up one dir, main page]

Skip to content

breaks with older github enterprise versions #794

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

Closed
electrofelix opened this issue Mar 15, 2018 · 12 comments
Closed

breaks with older github enterprise versions #794

electrofelix opened this issue Mar 15, 2018 · 12 comments

Comments

@electrofelix
Copy link
Contributor

Typically Github Enterprise is lagging keywords in the responses that are seen from github.com. Currently seeing the following error on trying to use this library with a local GitHub Enterprise instance.

IncompleteResponse [The library was expecting more data in the response (KeyError(u'license',)). Either GitHub modified it's response body, or your token is not properly scoped to retrieve this information.]

Thrown by a call to (github3.enterprise_login()).repository()

Patching the library to deal with this issue only results in seeing another issue:

IncompleteResponse: None The library was expecting more data in the response (KeyError(u'plan',)). Either GitHub modified it's response body, or your token is not properly scoped to retrieve this information.

Thrown by a call to (github3.enterprise_login()).me()

In both cases my token had full access, just that the public Github is returning more than the private versions.

One solution would be to update

try:
ret = response.json()
except ValueError:
raise exceptions.UnexpectedResponse(response)
to use something like the following:

        ret = collections.defaultdict(lambda: None)
        try:
            ret.update(response.json())
        except ValueError:
            raise exceptions.UnexpectedResponse(response)

But that would also require updating locations such as

self.plan = Plan(user['plan'], self)
to check if the value was not None before trying to use as an argument to create an object:

        if user['plan'] is not None:
            self.plan = Plan(user['plan'], self)
        else:
            self.plan = None

That would mean looking through each definition of _update_attributes() and modifying to test for None where the result of dictionary lookup was used to create an object.

This would certainly allow the library to work across a wide range of Github Enterprise releases, as well as simply returning None for certain data items when users scope tokens to have less permissions.

If this seems reasonable I can put together a PR to do the needful.

@sigmavirus24
Copy link
Owner

I was worried this would be the case. The problem we have, though, is that we're trying to provide a consistent interface for people. Previously an object would have None but None could have meant that the attribute wasn't present or that it was set to null and there was no way to distinguish between either. We tried experimenting with an Empty singleton but that proved incredibly confusing for users.

The worst part of this is that, as I understand it, there's nothing that would force a GitHubEnterprise to ever upgrade so we could potentially be supporting ancient versions for ever and that's very unappealing given the lack of funding by the corporate Enterprise users.

It's entirely plausible that supporting GitHubEnterprise will have to end.

An alternate would be to have a separate set of "Enterprise" objects that don't attempt to use attributes we know aren't there in the latest version. Sadly, however, there's 0 documentation for this.

@electrofelix
Copy link
Contributor Author

I should point out that the Github Enterprise instance I'm working with is 2.12 and was last updated this year. The problem is that new features added to the public one will take a few months to appear in even the most uptodate on premise installation.

However I think this goes beyond just the problem of dealing with older Github Enterprise instances, the current behaviour appears to prevent use of limited scope tokens because the error returned complains about not having sufficient scope. So any time a token does not have scope for information that results in a response will less information than expected, the user gets an exception.

When you talk about a singleton, you mean something like github3.Github.NotSet = object(), where any case where something was 'null' in the json response it is set to github3.Github.NotSet.

I guess the underlying question is, whether to support limited scope tokens or not? Because I believe that any solution for that, will also solve the issue for anyone using enterprise instances.

@guykisel
Copy link
guykisel commented Mar 16, 2018

If the github API fundamentally changes in a backwards-incompatible way, I think it's reasonable to stop supporting the old version of an API. However, if the github API just adds some new fields (such as license data) then I think the API wrapper should handle missing fields gracefully instead of just erroring.

As @electrofelix noted, GitHub Enterprise usually lags behind public GitHub releases by at least several months, so this will always be an issue.

In the meantime I just pinned my tool internally to github3.py==0.9.6 so at least there's an easy workaround :)

@electrofelix
Copy link
Contributor Author

Had some time to think about this and I think the solution might be to add a strict parameter defaulted to True to the GitHubCore object at

def __init__(self, json, session):
"""Initialize our basic object.
Pretty much every object will pass in decoded JSON and a Session.
"""

This would be used to control at

try:
ret = response.json()
except ValueError:
raise exceptions.UnexpectedResponse(response)
whether the decoded json is returned as a normal dictionary or one where missing settings return a github3.github.Empty object.

This would allow most users to continue without needing to know about github3.github.Empty, and those that are coding for Github Enterprise or looking to handle restricted scoped tokens can enable the strict=False behaviour. In such cases they would then need to understand the differences between github3.github.Empty and None.

This would still mean in a few places the code in this library would need to test for github3.github.Empty before trying to create an object such as

self.plan = Plan(user['plan'], self)

Might need to think a little on how to ensure strict it can be set on the initial connection, and then retained for any subsequent calls instead of needing to be set each time.

Does that strike the right balance between avoiding confusion for majority of users by not exposing such behaviour while ensuring those that may want to handle the more complex scenarios are also catered for?

@omgjlk
Copy link
Collaborator
omgjlk commented Mar 19, 2018

Some thoughts from a library user that needs to support both GHE and public GitHub. I think, given the nature of GitHub APIs and the fact that there can be N installs of Y versions of GitHub Enterprise out there, that we shouldn't be TOO strict on having keys populated with real data. I think it's reasonable that we can add the keys when Public API adds them, but allow the keys to be populated with None when the API we're talking to (like GHE installs) does not return a value. It DOES add some ambiguity to the response, but I think that's just a price that will be paid for supporting GHE at all.

It's unfortunate that Python doesn't really have a differentiation between "This value is not set" and "The value is empty/None", but I don't think it's worth inventing a new object to stuff in there to manufacture a difference.

I also think that introducing a strict option is overkill here too. I really think we just implement populating that data with None when we don't get a response and adding commentary around it in the source code to clarify what's happening, as well as some documentation that calls out GitHub Enterprise and the lagging API changes.

@sigmavirus24
Copy link
Owner

@omgjlk I think that's the most reasonable route forward. I'd like us to make only a select few attributes optional, however, to avoid ambiguity. There must be some subset of attributes that we can reliably present to the user. Either way this mess seems unavoidable, sadly.

Without a whole lot of experimentation and time that I simply do not have, this won't get done. As people can find and pick off areas where we need to be less stringent, I'll happily merge and release PRs. @omgjlk can release things too at this point, so hopefully that should help with the cadence here.

@omgjlk
Copy link
Collaborator
omgjlk commented Mar 20, 2018

Thanks @sigmavirus24 . I'll see if I can get access to some GHE installs to test against, but in the mean time I think we've got some data to move forward. I'll work on some patches in the next day or two.

@electrofelix
Copy link
Contributor Author

@sigmavirus24 Seems you are favouring being explicit as to which attributes are optional, so I'm wondering if altering the code at https://github.com/sigmavirus24/github3.py/blob/bf44c16510be84db7ec21cd7b2ae791626dc488c/github3/repos/repo.py#L2450-L245 to look like the following:

        self.original_license = repo.get('license', None)
        if self.original_license is not None:
            self.original_license = licenses.ShortLicense(
                self.original_license, self
)

Is reasonable?

Seems to align with the approach taken with the following blocks:

self.parent = repo.get('parent', None)
if self.parent is not None:
self.parent = ShortRepository(self.parent, self)
and
self.source = repo.get('source', None)
if self.source is not None:
self.source = ShortRepository(self.source, self)

@castillr
Copy link

Just another note.

github3.exceptions.IncompleteResponse: None The library was expecting more data in the response (KeyError(u'archived',)). Either GitHub modified it's response body, or your token is not properly scoped to retrieve this information.

We are running 2.11 so the archiving feature is missing. Does anyone know where the expected responses from GitHub Enterprise v3 are documented? I haven't been able to find it.

Back to v1.0.0a4 for me!

@omgjlk
Copy link
Collaborator
omgjlk commented Mar 20, 2018

The documentation would be at https://developer.github.com/enterprise/2.11/v3/

@sigmavirus24
Copy link
Owner

Just to clarify, I expect any attribute that is being made optional to have that reflected in the documentation. So for @electrofelix's example, I expect a corresponding change to the documentation that looks like:

.. attribute:: original_license

    This is the :class:`~github3.license.ShortLicense` returned as part of
    the repository. To retrieve the most recent license, see the
    :meth:`~github3.repos.repo.Repository.license` method.

Perhaps something like:

.. attribute:: original_license

    .. note::

        Under circumstances X, Y, and Z, this attribute will not be returned.
        To handle these situations sensitively, the attribute will be set to ``None``.
        Repositories may have a license associated with them in these cases.

    This is the :class:`~github3.license.ShortLicense` returned as part of
    the repository. To retrieve the most recent license, see the
    :meth:`~github3.repos.repo.Repository.license` method.

@castillr
Copy link

Interesting, https://developer.github.com/enterprise/2.11/v3/repos/#get states that "archived" is an expected key in the response. Don't know why our deployment isn't including it.

electrofelix added a commit to electrofelix/github3.py that referenced this issue Mar 21, 2018
Github Enterprise instances <= 2.12.7 do not include the license
attribute in responses for repository data. Allow this to be not set
and include a note in the documentation that the corresponding attr may
be set to None.

Partially fixes sigmavirus24#794
electrofelix added a commit to electrofelix/github3.py that referenced this issue Mar 21, 2018
Github Enterprise instances <= 2.12.7 do not include the license
attribute in responses for repository data. Allow this to be not set
and include a note in the documentation that the corresponding attr may
be set to None.

Partially
B494
 fixes sigmavirus24#794
electrofelix added a commit to electrofelix/github3.py that referenced this issue Mar 22, 2018
Github Enterprise instances <= 2.12.7 do not include the license
attribute in responses for repository data. Allow this to be not set
and include a note in the documentation that the corresponding attr may
be set to None.

Partially fixes sigmavirus24#794
electrofelix added a commit to electrofelix/github3.py that referenced this issue Mar 22, 2018
Github Enterprise instances <= 2.12.7 do not include the license
attribute in responses for repository data. Allow this to be not set
and include a note in the documentation that the corresponding attr may
be set to None.

Partially fixes sigmavirus24#794
electrofelix added a commit to electrofelix/github3.py that referenced this issue Mar 26, 2018
Github Enterprise instances <= 2.12.7 do not include the `plan`
attribute in responses for authenticated user data. Allow this to be
not set and include a note in the documentation that the corresponding
attr may be set to `None`.

Partially fixes sigmavirus24#794
electrofelix added a commit to electrofelix/github3.py that referenced this issue Mar 30, 2018
Github Enterprise instances <= 2.12.7 do not include the `plan`
attribute in responses for authenticated user data. Allow this to be
not set and include a note in the documentation that the corresponding
attr may be set to `None`.

Partially fixes sigmavirus24#794
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

No branches or pull requests

5 participants
0