-
Notifications
You must be signed in to change notification settings - Fork 406
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
Comments
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 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. |
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 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. |
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 |
Had some time to think about this and I think the solution might be to add a Lines 28 to 33 in bf44c16
This would be used to control at Lines 160 to 163 in bf44c16
github3.github.Empty object.
This would allow most users to continue without needing to know about This would still mean in a few places the code in this library would need to test for Line 757 in bf44c16
Might need to think a little on how to ensure 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? |
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 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 |
@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. |
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. |
@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:
Is reasonable? Seems to align with the approach taken with the following blocks: github3.py/github3/repos/repo.py Lines 2458 to 2460 in bf44c16
github3.py/github3/repos/repo.py Lines 2463 to 2465 in bf44c16
|
Just another note.
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! |
The documentation would be at https://developer.github.com/enterprise/2.11/v3/ |
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:
Perhaps something like:
|
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. |
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
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
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
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
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
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
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.
Thrown by a call to (github3.enterprise_login()).repository()
Patching the library to deal with this issue only results in seeing another issue:
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
github3.py/github3/models.py
Lines 160 to 163 in bf44c16
But that would also require updating locations such as
github3.py/github3/users.py
Line 757 in bf44c16
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.
The text was updated successfully, but these errors were encountered: