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

GitLab Integration #3327

Merged
merged 43 commits into from Dec 7, 2017
Merged

GitLab Integration #3327

merged 43 commits into from Dec 7, 2017

Conversation

@humitos
Copy link
Member

@humitos humitos commented Nov 28, 2017

This is the continuation of the work done by @saily at #1870

I updated the code to follow the new codebase and also use the apiv4 endpoints from gitlab. Besides, there are a bunch of style-things.

Missing things:

  • update webhook method
  • more testing
  • send proper context to the sphinx theme

Closes #1441
Closes #1870
Closes #1443

Daniel Widerin and others added 25 commits Jun 16, 2016
Support private repositories and cleanup
…ocs.org into saily-sync-gitlab-repos

- Skip test_oauth.py from flake8 since it has a lot of "Too long
  lines"
- Add some style fixes following the new rules
- Minimum adaptation for the registry OAuth classes
@@ -26,6 +26,7 @@ repos:
'flake8-string-format',
'flake8-tidy-imports',
'flake8-todo']
exclude: 'test_oauth.py'
Copy link
Member Author

@humitos humitos Nov 28, 2017

This file has a lot of long lines that I don't want to fix now :)


* Go to the **Settings** page for your project
* Click **Integrations**
* For **URL**, use the URL of the integration on Read the Docs, found on the
:ref:`integration detail page <integration-detail>` page
* Leave the default **Push events** selected
* Leave the default **Push events** selected and mark **Tag push events** also
Copy link
Member Author

@humitos humitos Nov 28, 2017

Not sure, but I think we need the Tag push events here also...

Copy link
Member

@ericholscher ericholscher Nov 29, 2017

Seems like we'd want them. +1

# TODO: I think this can be different than `gitlab.com`
# self.adapter.provider_base_url
GL_REGEXS = [
re.compile('gitlab.com/(.+)/(.+)(?:\.git){1}'),
Copy link
Member Author

@humitos humitos Nov 28, 2017

Are we going to support private gitlab instalation? How this should looks like?

Copy link
Contributor

@agjohnson agjohnson Nov 28, 2017

We will, but this integration will be on the .com, we won't put that code here yet.

Copy link
Member Author

@humitos humitos Nov 28, 2017

OK. So, I think the TODO note is good for now. I will leave it there

re.escape(urlparse(adapter.provider_base_url).netloc))
default_avatar = {
'repo': urljoin(settings.MEDIA_URL, 'images/fa-bookmark.svg'),
'org': urljoin(settings.MEDIA_URL, 'images/fa-users.svg'),
Copy link
Member Author

@humitos humitos Nov 28, 2017

Do we want these default avatars? Github and Bitbucket are not using nothing similar to this.

Copy link
Contributor

@agjohnson agjohnson Nov 28, 2017

I'd say no, we use the github/bitbucket supplied avatar as the only source I believe.

or, alternatively, we edit all to include a default avatar. Perhaps in another PR later?

Copy link
Member Author

@humitos humitos Nov 28, 2017

I will remove the default avatars and its files.

We can make it in another PR for all the services, but I think it doesn't worth it really.


# The ID or URL-encoded path of the project
# https://docs.gitlab.com/ce/api/README.html#namespaced-path-encoding
repo_id = '{}%2F{}'.format(owner, repo)
Copy link
Member Author

@humitos humitos Nov 28, 2017

This is enough here but maybe it's better to find a way to get the proper repo:id from gitlab

Copy link
Contributor

@agjohnson agjohnson Nov 28, 2017

this is probably fine. Github gives us this information in the api return, but i don't see anything wrong with this as long as owner and repo are supplied from the API.

Copy link
Member Author

@humitos humitos Nov 28, 2017

I found a better way to do this,

        # The ID or URL-encoded path of the project
        # https://docs.gitlab.com/ce/api/README.html#namespaced-path-encoding
        repo_id = json.loads(project.remote_repository.json).get('id')

It uses the RemoteRepository response saved in our database, which I think it's better than parsing a URL, more specifically in GitLab that the URL could have different domains.

So, I'll try to use the repo id here instead of owner + repo and we can get rid of the GL_REGEX also, since it won't be needed anymore.

Copy link
Member Author

@humitos humitos Nov 28, 2017

The regex is also used at this point:

https://github.com/rtfd/readthedocs.org/blob/4db7339aac7a52d7a35b6359eff8032e8aadf6ae/readthedocs/doc_builder/backends/sphinx.py#L72-L75

So, we will need to figure this out for that case, at least.

pass

# @classmethod
# def get_token_for_project(cls, project, force_local=False):
Copy link
Member Author

@humitos humitos Nov 28, 2017

I think this is only needed by Github, but I don't understand how it works yet...

'gitlab': {
'SCOPE': [
'api',
'read_user',
Copy link
Member Author

@humitos humitos Nov 28, 2017

Not sure, but maybe the sudo permission is also required.

Copy link
Contributor

@agjohnson agjohnson Nov 28, 2017

We'll want to make sure here. Folks are really particular about the permissions we request

Copy link
Member Author

@humitos humitos Nov 28, 2017

I'm not sure to understand their documentation. Seems poor to me: https://docs.gitlab.com/ce/user/profile/personal_access_tokens.html#limiting-scopes-of-a-personal-access-token

So far so good, I was able to connect the account and setup the webhooks using only these two.

@@ -228,15 +231,6 @@ def INSTALLED_APPS(self): # noqa
CELERY_CREATE_MISSING_QUEUES = True

CELERY_DEFAULT_QUEUE = 'celery'
# Wildcards not supported: https://github.com/celery/celery/issues/150
CELERY_ROUTES = {
Copy link
Member Author

@humitos humitos Nov 28, 2017

I think this is not used anymore. None of those tasks exist.

Copy link
Contributor

@agjohnson agjohnson Nov 28, 2017

We've moved to marking the queue on @task declaration

@humitos
Copy link
Member Author

@humitos humitos commented Nov 29, 2017

Just a comment about our auto-linting configuration:

@agjohnson I would say that we should disable D202 since I didn't find a way to make it automatically with the tools that I know. Having it enabled will produce a lot of this kind of manual changes: 0ce1702

The other way around is supported by yapf and added automatically: making that space mandatory, c2f16d6#diff-4d8c13c2356e3f836af6d1cbfd975203

@humitos
Copy link
Member Author

@humitos humitos commented Nov 29, 2017

@agjohnson another thing that it's missing here (but probably for another PR that also change the theme) is the context sent to sphinx at https://github.com/rtfd/readthedocs.org/blob/4db7339aac7a52d7a35b6359eff8032e8aadf6ae/readthedocs/doc_builder/backends/sphinx.py#L112

Let me know how you prefer to work on that.

@humitos humitos requested a review from ericholscher Nov 29, 2017
Copy link
Member

@ericholscher ericholscher left a comment

Looks great to me! I didn't go through the gitlab docs really to see if we're doing everything right, but the code looks great. 👍

@@ -13,7 +13,7 @@ More information can be found in the :doc:`vcs` page.
Auto-updating
-------------

The :doc:`webhooks` page talks about the different ways you can ping RTD to let us know your project has been updated. We have official support for GitHub, and anywhere else we have a generic post-commit hook that allows you to POST to a URL to get your documentation built.
The :doc:`webhooks` page talks about the different ways you can ping RTD to let us know your project has been updated. We have official support for GitHub, Bitbucket and GitLab, and anywhere else we have a generic post-commit hook that allows you to POST to a URL to get your documentation built.
Copy link
Member

@ericholscher ericholscher Nov 29, 2017

<3 doc updates.

Copy link
Member Author

@humitos humitos Nov 29, 2017

Credits to @saily, the original author :)


* Go to the **Settings** page for your project
* Click **Integrations**
* For **URL**, use the URL of the integration on Read the Docs, found on the
:ref:`integration detail page <integration-detail>` page
* Leave the default **Push events** selected
* Leave the default **Push events** selected and mark **Tag push events** also
Copy link
Member

@ericholscher ericholscher Nov 29, 2017

Seems like we'd want them. +1

"""
# TODO Replace this check by keying project to remote repos
return (
cls.url_pattern is not None and
cls.url_pattern.search(project.repo) is not None
)
cls.url_pattern.search(project.repo) is not None)
Copy link
Member

@ericholscher ericholscher Nov 29, 2017

All these are linting changes, I think? This is a bit of what I was worried about w/ the auto-linting, making it hard to catch real changes in the massive linting changes.

Copy link
Member Author

@humitos humitos Nov 29, 2017

Yes, they are linting changes.

I try to make these changes in separated commits but sometimes it's complicated.

I don't know what's the best way for this. I think we can make a huge PR with all the automcatic linting changes and fix the specific one by hand after merging it. That will make PR with linting changes smaller in the future.

But I don't know what's best, honestly (I'm used to review PR with a lot of linting changes because we followed the same as I'm doing here but...)

Copy link
Contributor

@agjohnson agjohnson Nov 29, 2017

We discussed this a little. I don't think large commit is the answer yet, this shouldn't be priority. We can however be more careful in making PRs where autolint is a separate commit. This should be more easy to review, but requires some careful git rebasing depending on your workflow (if you are commiting autolint changes with your logic changes)

Or just a separate PR, whatever is easier to manage for your own sake.

log.debug(
'Not importing %s because mismatched orgs',
fields['name'],
)
Copy link
Member

@ericholscher ericholscher Nov 29, 2017

Wonder if this should be a higher priority? Will this cause breakage for the user?

Copy link
Member Author

@humitos humitos Nov 29, 2017

Honestly. I'm not sure. Github and Bitbucket code do exactly the same.

It's kind of a weird case, because it should happend that user U created the repo R with organization O. Then, user UU tries to import the same repo R but with organization OO and that generates this error, right?

Copy link
Contributor

@agjohnson agjohnson Nov 29, 2017

That is correct. Probably no need to surface this any harder, it's a weird edge case.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Nov 29, 2017

another thing that it's missing here

@humitos could you explain this more, i don't quite understand

re: #3327 (comment)

@humitos
Copy link
Member Author

@humitos humitos commented Nov 29, 2017

@agjohnson we are sending some context to sphinx (github_user, github_repo, etc). So, we will to send the same information about gitlab_ and the theme should have to accept it and do the magic to add the Edit in Gitlab link, and more.

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Nov 29, 2017

No need to do this on signal, it can be included in the block above, similar to github/butbucket. But good point, i didn't realize this was missing.

@humitos
Copy link
Member Author

@humitos humitos commented Nov 29, 2017

@agjohnson
Copy link
Contributor

@agjohnson agjohnson commented Dec 1, 2017

Looks like a few more pieces missing. I'll assign this to @humitos as a WIP until it's wrapped up next week.

Copy link
Contributor

@agjohnson agjohnson left a comment

Okay ... :)

One last change, we just need a test case for this GitLab URL in our context data. With that, I swear this will be mergeable.

path=filename,
source_suffix=source_suffix,
action=action_string,
)
Copy link
Contributor

@agjohnson agjohnson Dec 5, 2017

We should test this method

Copy link
Member Author

@humitos humitos Dec 6, 2017

Done!

@agjohnson agjohnson merged commit ba71aa1 into master Dec 7, 2017
2 checks passed
@agjohnson agjohnson deleted the humitos/gitlab/integration branch Dec 7, 2017
@fcasson
Copy link

@fcasson fcasson commented May 21, 2020

Can anyone advise how to customise the gitlab_url - we have our own deployment. Following https://sphinx-rtd-theme.readthedocs.io/en/stable/configuring.html#file-wide-metadata, I tried to set in conf.py

html_context = {
"display_gitlab": True, # Integrate Gitlab
"gitlab_user": "myuser", # Organization or User
"gitlab_repo": "myrepo", # Repo name
"gitlab_version": "develop", # Branch
"conf_py_path": "/webpages/wiki/", # Path in the checkout to the docs root
"gitlab_url": "https://git.mylocalgitlab.org.uk/"
}

But it doesn't work, it remains set to gitlab.com in the HTML. Did I put this in the wrong place?

EDIT: Fixed. The right variable to use in html_context settings is gitlab_host (I inferred this from snippet on https://docs.readthedocs.io/en/stable/guides/vcs.html though it is not documented explicitly)

@humitos
Copy link
Member Author

@humitos humitos commented May 25, 2020

@fcasson please, next time open a new issue instead of commenting in an old PR. Thanks!

I think you will need DONT_OVERWRITE_SPHINX_CONTEXT set in your project. See https://docs.readthedocs.io/en/stable/guides/feature-flags.html#feature-flags --let me know (via email support) if that makes sense in your case.

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

Successfully merging this pull request may close these issues.

7 participants