Github Enterprise Support #3853

gaborbernat · 2018-03-26T16:20:04Z

This solves #2194.

This change is

humitos

Thanks for your contribution! This seems to be a good addition.

Although, I'm not sure to fully understand the problem itself. The only difference between GH and GHE is the URL that we hit? I mean, instead of github.com we need to hit whatever you defined in {{github_url}}?

It seems that it's needed to update a django setting to match the github provider name to the enterprise URL as pointed here: http://django-allauth.readthedocs.io/en/latest/providers.html#enterprise-support

I did that change and it worked. So, I think we need to mention that at some point.

In [1]: from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter

In [2]: GitHubOAuth2Adapter.profile_url
Out[2]: 'http://localhost:8000/api/v3/user'

In [3]:

Finally, is there a way to co-exist both connections here: GH and GHE?

I'm requesting these changes before merging, but I think it's pretty close.

humitos · 2018-03-28T14:42:27Z

docs/custom_installs/github_enterprise.rst

+To swap out this connection with an enterprise edition of Github you need to:
+
+1. Create an OAuth Github Application by following the `official guide <https://developer.github.com/apps/building-oauth-apps/creating-an-oauth-app/>`_. Note here you must provide the address of the readthedocs
+instance you're setting up. This will give you a Github application ``client id`` and ``secret``.


Shouldn't this line be indented at the same level than Create word?

humitos · 2018-03-28T14:43:36Z

readthedocs/builds/models.py

@@ -36,6 +38,8 @@

 DEFAULT_VERSION_PRIVACY_LEVEL = getattr(
    settings, 'DEFAULT_VERSION_PRIVACY_LEVEL', 'public')
+ARGS = '/{user}/{repo}/{action}/{version}{docroot}{path}{source_suffix}'


maybe a name like _GITHUB_URL_ARS instead?

humitos · 2018-03-28T15:01:42Z

docs/custom_installs/github_enterprise.rst

+        site, _ = Site.objects.update_or_create(id=SITE_ID, defaults=dict(name=PRODUCTION_DOMAIN, domain=PRODUCTION_DOMAIN))
+
+        # change the github social account provider to link to the enterprise Github
+        GITHUB = "{{github_url}}"


This doesn't seem to be used at all.

gaborbernat · 2018-03-28T15:38:31Z

@humitos yes you need to change that, however in some places the github.com was hard coded into the code, I've replaced in these cases to instead read it from from allauth.socialaccount.providers.github.provider import GitHubProvider (also I've already mention you need to do the change in there at: https://github.com/gaborbernat/readthedocs.org/blame/38ee82afe90b2a0af455f93b7baa699db4b8cf50/docs/custom_installs/github_enterprise.rst#L32 - this is a config change)
We could expose a main level variable in config if you guys think it would make it easier.

To have the connect account feature work you need to setup the site setting correctly (e.g. point to your domain and not example.com as it's by default - which is the first change in the script).

As for having both exists do we have a strong need for that, as would probably complicate the implementation.

gaborbernat · 2018-03-28T17:17:09Z

@humitos made the requested changes, ready to discuss supporting an enterprise instance with github.com in parallel.

xrmx · 2018-03-28T19:58:02Z

readthedocs/builds/models.py

@@ -5,6 +5,8 @@
    absolute_import, division, print_function, unicode_literals)

 import logging
+
+from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter


from should go after import, and third party import should go after stdlib import and before local imports

for import sorting, you can run isort command which does the trick automatically following our rules.

xrmx · 2018-03-28T19:58:18Z

readthedocs/core/management/commands/import_github_language.py

@@ -37,17 +40,12 @@ def handle(self, *args, **options):
            return

        for project in Project.objects.filter(
-            programming_language__in=['none', '', 'words']
+                programming_language__in=['none', '', 'words']


4 spaces was fine

xrmx · 2018-03-28T19:59:13Z

tox.ini

@@ -15,6 +15,7 @@ setenv =
    DJANGO_SETTINGS_MODULE=readthedocs.settings.test
    LANG=C
    DJANGO_SETTINGS_SKIP_LOCAL=True
+passenv = https_proxy http_proxy no_proxy


looks unrelated

xrmx · 2018-03-28T19:59:44Z

readthedocs/core/management/commands/import_github_language.py

@@ -11,13 +11,16 @@
 from __future__ import absolute_import
 from __future__ import print_function
 import os
+
+from allauth.socialaccount.providers.github.views import GitHubOAuth2Adapter


same comment as before

gaborbernat · 2018-03-29T09:34:43Z

Made the requested changes. By the way running isort would modify a lot of existing source files 🤔 are we sure it's configured correctly?

humitos

Left some minor comments

humitos · 2018-03-29T15:59:35Z

docs/custom_installs/github_enterprise.rst

+
+    .. code-block:: python
+
+        SOCIALACCOUNT_PROVIDERS = {'github': {'SCOPE': ['user:email', 'read:org',


Can you update the formatting style here to be similar like this: https://github.com/rtfd/readthedocs.org/blob/adb5392c47a1b3fe506524d898dd147b038d4140/readthedocs/settings/base.py#L273-L289 ?

Also, if you modify local_settings.py like this, you override the setting and loose the gitlab feature, for example.

I've been using somethin like this:

from .dev import CommunityDevSettings INSTALLED_APPS = CommunityDevSettings().INSTALLED_APPS + [ 'debug_toolbar', ]

okay I can do; my approach was that people in an enterprise env usually just want one VCS, hence why removing gitlab is not needed anymore

humitos · 2018-03-29T16:15:19Z

readthedocs/projects/constants.py

-    re.compile('github.com/(.+)/(.+)'),
-    re.compile('github.com:(.+)/(.+)\.git$'),
+GITHUB_REGEX_BASES = [
+    '{}/(.+)/(.+)(?:\.git){{1}}$',


Not sure if this was on purpose, but it was {1} and now it's {{1}}. I suppose the original was correct.

no this change is needed, because now we'll format the string to get the regex, hence the double quotation actually keeps it as it was beforehand (aka literal {)

humitos · 2018-03-29T16:19:37Z

Made the requested changes. By the way running isort would modify a lot of existing source files thinking are we sure it's configured correctly?

Yes. It's correctly configured but it's something new and we didn't run to the entire project yet. We were running it only the files that we modify on each PR.

So, you could run it only on those files.

At a higher level, we have pre-commit configured (which will run only on your modified fields) but we are not forcing the community PR to run this command yet.

humitos · 2018-03-29T16:24:38Z

As for having both exists do we have a strong need for that, as would probably complicate the implementation.

We don't really need this for the community site since we don't support GHE in .org but I was asking you to know if this could be applicable in the readthedocs.com site. There, we will need to support:

multiples GHE accounts
GHE and regular GH connections living together
accessing to a private network of GHE instace

Anyway, I was checking if we could advance in one of those point in this PR but it seems that we won't be able to do this yet.

Currently, this is only useful for private installations of RTD source code.

Changes I requested were already done

gaborbernat · 2018-03-29T16:46:36Z

@humitos tox organization has a fix-lint target to setup and run automatically the pre-commit maybe we should have it here too, but that's part of a different PR/issue

Indeed this PR is useful for private installations of RTD. We can look into supporting more part of further subsequent PRs. I don't have access to multiple instances of these, hence why was not focusing on taking a look at it. But most likely it's something that can be done.

Additionally made the required change (this way we keep previous settings - gitlab included, only upgrade the github URL):

from .dev import CommunityDevSettings
_SETTINGS = CommunityDevSettings()

SOCIALACCOUNT_PROVIDERS = _SETTINGS.SOCIALACCOUNT_PROVIDERS
SOCIALACCOUNT_PROVIDERS['github']['GITHUB_URL'] == '{{github_url}}'

Part of a future PR we may want to make the GITHUB_URL a root level settings, and the CommunityDevSettings use that to construct it's own SOCIALACCOUNT_PROVIDERS.

PS. Also did rebase on current master, hence the change in commit hash here.

…l remove the github.com integration

humitos · 2018-03-29T22:38:56Z

readthedocs/oauth/services/github.py

@@ -28,7 +28,7 @@ class GitHubService(Service):

    adapter = GitHubOAuth2Adapter
    # TODO replace this with a less naive check
-    url_pattern = re.compile(r'github\.com')
+    url_pattern = re.compile(re.escape(GitHubOAuth2Adapter.web_url))


I just realized that we can simply use self.adapter in all this class.

(it's the common usage for all the services implemented in RTD)

gaborbernat · 2018-04-05T11:32:30Z

@humitos done

humitos · 2018-04-05T14:03:25Z

@gaborbernat nice! Thanks. There is a linting error, could you check it?

To me, this PR is ready for merge. Let's wait for another core team member to review it -- @agjohnson was interested on these changes also.

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>

gaborbernat · 2018-04-05T15:22:59Z

@humitos fixed 😄

gaborbernat · 2018-04-09T17:01:50Z

@core ping 👍

RichardLitt · 2018-04-09T18:36:24Z

You're heard, @gaborbernat. Four days (and over the weekend) isn't a lot of time for us - we'll get to this when we can.

agjohnson · 2018-04-17T23:55:52Z

I'm happy that someone is able to take this on. Thanks for the contribution @gaborbernat !

I'll have more feedback when I can focus on this more, but for starters: this is touching a lot of important code and has no tests. So, fixing that is a good place to continue this work.

I'm going to target a future release, as this isn't quite on our roadmap. I think this is mostly close though. Apologies for any delays responding, off-roadmap PRs have lower priority for us.

gaborbernat · 2018-06-12T09:50:14Z

Just an update. I definitely plan to continue to work on this. Just other things have come up. The fact that the actual code has not tests at the moment means I first need to figure out how this part is intended to be tested. Which makes picking up this non-trivial.

gaborbernat · 2019-01-09T16:20:25Z

Due to lack of resource for working on this, I'll need to retract this PR. Thanks for the collaboration guys.

RichardLitt · 2019-01-09T17:59:30Z

@gaborbernat Thanks for letting us know, and for your work, even if it wasn't merged in the end. :)

gaborbernat force-pushed the master branch 6 times, most recently from 888a0b0 to 38ee82a Compare March 27, 2018 08:46

ericholscher requested a review from a team March 28, 2018 10:42

humitos added the PR: ready for review label Mar 28, 2018

humitos previously requested changes Mar 28, 2018

View reviewed changes

gaborbernat force-pushed the master branch from c026eed to b0aa11e Compare March 28, 2018 17:17

xrmx reviewed Mar 28, 2018

View reviewed changes

RichardLitt added the Improvement Minor improvement to code label Mar 28, 2018

gaborbernat force-pushed the master branch from b0aa11e to 6ae1f32 Compare March 29, 2018 09:33

humitos reviewed Mar 29, 2018

View reviewed changes

gaborbernat added 2 commits March 29, 2018 17:52

GHE support

de61715

keep previous setting in GHE connection guide, and point out that wil…

3c5f267

…l remove the github.com integration

gaborbernat force-pushed the master branch from f5131df to 3c5f267 Compare March 29, 2018 16:52

humitos reviewed Mar 29, 2018

View reviewed changes

use adapter instead of explicit GithubOAuth2Adapter

6cd6f24

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>

gaborbernat force-pushed the master branch from c6c1aaf to 6cd6f24 Compare April 5, 2018 14:59

agjohnson added Needed: tests Tests are required PR: work in progress Pull request is not ready for full review and removed PR: ready for review labels Apr 17, 2018

agjohnson added this to the 2.5 milestone Apr 17, 2018

gaborbernat mentioned this pull request Jun 12, 2018

[WIP] Format code with black pypa/pip#5425

Closed

gaborbernat mentioned this pull request Sep 19, 2018

Cannot link private enterprise github account and import repository #2194

Closed

agjohnson modified the milestones: 2.10, 2.11 Nov 10, 2018

gaborbernat closed this Jan 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Github Enterprise Support #3853

Github Enterprise Support #3853

gaborbernat commented Mar 26, 2018 •

edited

Loading

humitos left a comment

humitos Mar 28, 2018

humitos Mar 28, 2018

humitos Mar 28, 2018

gaborbernat commented Mar 28, 2018

gaborbernat commented Mar 28, 2018

xrmx Mar 28, 2018

humitos Mar 28, 2018

xrmx Mar 28, 2018

xrmx Mar 28, 2018

xrmx Mar 28, 2018

gaborbernat commented Mar 29, 2018

humitos left a comment

humitos Mar 29, 2018

humitos Mar 29, 2018

gaborbernat Mar 29, 2018

humitos Mar 29, 2018

gaborbernat Mar 29, 2018

humitos commented Mar 29, 2018

humitos commented Mar 29, 2018

gaborbernat commented Mar 29, 2018 •

edited

Loading

humitos Mar 29, 2018

gaborbernat commented Apr 5, 2018

humitos commented Apr 5, 2018

gaborbernat commented Apr 5, 2018

gaborbernat commented Apr 9, 2018

RichardLitt commented Apr 9, 2018

agjohnson commented Apr 17, 2018

gaborbernat commented Jun 12, 2018

gaborbernat commented Jan 9, 2019

RichardLitt commented Jan 9, 2019


		.. code-block:: python

		SOCIALACCOUNT_PROVIDERS = {'github': {'SCOPE': ['user:email', 'read:org',

Github Enterprise Support #3853

Github Enterprise Support #3853

Conversation

gaborbernat commented Mar 26, 2018 • edited Loading

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat commented Mar 28, 2018

gaborbernat commented Mar 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gaborbernat commented Mar 29, 2018

humitos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

humitos commented Mar 29, 2018

humitos commented Mar 29, 2018

gaborbernat commented Mar 29, 2018 • edited Loading

Choose a reason for hiding this comment

gaborbernat commented Apr 5, 2018

humitos commented Apr 5, 2018

gaborbernat commented Apr 5, 2018

gaborbernat commented Apr 9, 2018

RichardLitt commented Apr 9, 2018

agjohnson commented Apr 17, 2018

gaborbernat commented Jun 12, 2018

gaborbernat commented Jan 9, 2019

RichardLitt commented Jan 9, 2019

gaborbernat commented Mar 26, 2018 •

edited

Loading

gaborbernat commented Mar 29, 2018 •

edited

Loading