[WIP] GitLab addon by danielneis · Pull Request #5929 · CenterForOpenScience/osf.io

danielneis · 2016-07-06T13:09:53Z

Purpose

Implement the GitLab addon according to the GitLab Integration Grant Spec at https://gist.github.com/felliott/cc3d48a492c174545b7f

Changes

"This add-on will allow users to specify a GitLab instance to communicate with, and then connect their GitLab account to their OSF account via an OAuth token. Users may then connect a project for which they have admin rights to one of their GitLab-hosted repositories. Users should also be able to change which repository is linked to a given project at any time and revoke permissions from any or all projects as desired."

Side effects

$ cat website/addons/gitlab/requirements.txt
cachecontrol==0.10.2
pyapi-gitlab==7.8.5
python-magic==0.4.6

Roadmap

mfraezz

A quick set of drive-by comments. Looking pretty good, but I have a few questions about whether or not everything copied from GitHub code is applicable to GitLab.

Please also get the TCI build passing.

Pass complete

mfraezz · 2016-11-30T19:50:49Z

website/addons/gitlab/__init__.py

+
+# Note: Even though GitLab supports file sizes over 1 MB, uploads and
+# downloads through their API are capped at 1 MB.
+MAX_FILE_SIZE = 100


If the above is correct, this should be 1, not 100

mfraezz · 2016-11-30T19:52:32Z

website/addons/gitlab/api.py

+    def user_repos(self, user):
+        return self.gitlab.getprojectsowned()
+
+    def my_org_repos(self, permissions=None):


Other than being mocked in tests, is this intended to be used anywhere?

mfraezz · 2016-11-30T19:53:03Z

website/addons/gitlab/api.py

+
+        :param str repo_id: GitLab repository id
+        :return: Dict of repo information
+            See #TODO: link gitlab docs


Please resolve TODO

mfraezz · 2016-11-30T19:54:25Z

website/addons/gitlab/api.py

+            user if omitted
+        :return dict: GitLab API response
+        """
+        return self.gitlab.currentuser()


Here and elsewhere, if this outgoing request fails (e.g, due to invalid credentials), are any errors being handled gracefully?

mfraezz · 2016-11-30T19:56:59Z

website/addons/gitlab/api.py

+        :return: List of branch dicts
+            http://developer.github.com/v3/repos/#list-branches
+        """
+        # TODO


TODO what?

mfraezz · 2016-11-30T20:46:16Z

website/addons/gitlab/templates/gitlab_user_settings.mako

+
+    <h4 class="addon-title">
+        <img class="addon-icon" src=${addon_icon_url}>
+        <span data-bind="text: properName"></span> <!-- TODO: Can we use mako addon_full_name as some other addons do? -->


TODO: Can we use mako addon_full_name

Probably, yes.

mfraezz · 2016-11-30T20:46:46Z

website/addons/gitlab/tests/test_views.py

+
+
+# TODO: Test remaining CRUD methods
+# TODO: Test exception handling


Should these be done?

mfraezz · 2016-11-30T20:47:07Z

website/addons/gitlab/tests/utils.py

+        settings.repo = 'abc'
+        settings.user = 'octo-cat'
+
+# TODO: allow changing the repo name


Is this necessary?

mfraezz · 2016-11-30T20:48:27Z

website/addons/gitlab/tests/test_views.py

+        self.node_settings.owner.is_registration = True
+        assert_false(check_permissions(self.node_settings, self.consolidated_auth, connection, 'master'))
+
+    def check_hook_urls(self, urls, node, path, sha):


This probably won't be found by nosetests because the function name doesn't begin with test_

it is used by tests on https://github.com/CenterForOpenScience/osf.io/pull/5929/files/1fbc939831c5994ea0088cd0303b11ce45868563#diff-7bf832417d62003700f5f44838fc208aR289 and https://github.com/CenterForOpenScience/osf.io/pull/5929/files/1fbc939831c5994ea0088cd0303b11ce45868563#diff-7bf832417d62003700f5f44838fc208aR316

mfraezz · 2016-11-30T20:49:17Z

website/addons/gitlab/views.py

+    urls = {}
+
+    if include_urls:
+        # TODO: Move to helper function


Is this necessary? It looks like the only other place it's happening is in a test, so I'd probably say it's not.

felliott

Hey @danielneis and @rafaeldelucena!

I'm sending this back for another pass. I hit a few issues trying to run it locally, most of which I think were caused by the Fangorn GitLab root object not having a .data.extra field. I've also made a note of which pieces can be removed in order to target a read-only provider first. We can add that back in after the read-only version is released. The other bit that will need updating when going read-only is the file editor that is constructed when viewing a file on the OSF. See this line from my Bitbucket PR.

Cheers,
@felliott

felliott · 2017-03-10T21:47:30Z

website/addons/gitlab/model.py

+    repo_id = fields.StringField()
+    hook_id = fields.StringField()
+    hook_secret = fields.StringField()
+    registration_data = fields.DictionaryField()


While working on the Bitbucket addon (also based off GitHub) I noticed this field registration_data and some code below that didn't make sense. It turns out this is code from a loooooooong, long time ago that doesn't do anything anymore and can be removed.

felliott · 2017-03-10T21:47:46Z

website/addons/gitlab/model.py

+        self.repo_id = None
+        self.hook_id = None
+        self.hook_secret = None
+        self.registration_data = None


See above. This can be deleted.

felliott · 2017-03-10T22:06:37Z

website/addons/gitlab/static/gitlabFangornConfig.js

+var m = require('mithril');
+var $ = require('jquery');
+var URI = require('URIjs');
+var Fangorn = require('js/fangorn');


When you bring this up-to-date with develop-backup, you'll need to change this to var Fangorn = require('js/fangorn').Fangorn;

felliott · 2017-03-10T22:10:05Z

website/addons/gitlab/static/gitlabFangornConfig.js

+        var buttons = [];
+        function _downloadEvent(event, item, col) {
+            event.stopPropagation();
+            var branch = tem.data.extra.ref || item.data.extra.fileSha;


typo: tem.data.extra.ref => item.data.extra.ref

If item is the root folder, it won't necessarily have a data.extra field. You'll need to check if it exists first. If it doesn't you can fallback to item.data.default_branch.

felliott · 2017-03-10T22:49:58Z

website/addons/gitlab/static/gitlabLogActionList.json

@@ -0,0 +1,10 @@
+{
+  "gitlab_file_added" : "${user} added file ${path} to GitLab repo ${repo} in ${node}",


The ${repo} variable in this log message comes from website/static/js/logTextParser.js. That function is written specifically for github, so you'll need to add your own ${gitlab_repo} variable. I did the same thing for Bitbucket, you can see the examples here, here, and here.

felliott · 2017-03-14T21:20:49Z

website/addons/gitlab/static/gitlabFangornConfig.js

+// Cross browser key codes for the Command key
+var commandKeys = [224, 17, 91, 93];
+
+function _uploadUrl(item, file) {


Not needed for read-only.

felliott · 2017-03-14T21:21:00Z

website/addons/gitlab/static/gitlabFangornConfig.js

+    return waterbutler.buildTreeBeardUpload(item, file, {branch: branch });
+}
+
+function _removeEvent (event, items) {


Not needed for read-only

felliott · 2017-03-14T21:22:16Z

website/addons/gitlab/static/gitlabFangornConfig.js

+            }
+            if (tb.options.placement !== 'fileview') {
+                // If File and FileRead are not defined dropzone is not supported and neither is uploads
+                if (window.File && window.FileReader && item.data.permissions && item.data.permissions.edit) {


The 'Upload', 'Create Folder', and 'Delete' buttons can be removed from the read-only addon.

felliott · 2017-03-14T21:22:46Z

website/addons/gitlab/static/gitlabFangornConfig.js

+                    }, 'View'));
+            }
+            if (item.data.permissions && item.data.permissions.edit) {
+                buttons.push(


This button can be removed from the read-only provider.

felliott · 2017-03-14T21:23:10Z

website/addons/gitlab/static/gitlabFangornConfig.js

+            }
+        }
+
+        if(item.data.provider && !item.data.isAddonRoot && item.data.permissions && item.data.permissions.edit && tb.options.placement !== 'fileview') {


'Rename' can be removed from the read-only provider.

mfraezz · 2017-07-03T21:49:51Z

Closing in favor of #7418 , consider this merged. Thanks for all your hard work, @danielneis , @luismulinari , and @rafaeldelucena .

danielneis force-pushed the feature/gitlab branch from f20f58e to bb6542c Compare August 19, 2016 14:22

danielneis force-pushed the feature/gitlab branch from 51beba5 to 5f601e6 Compare November 29, 2016 18:33

mfraezz requested changes Nov 30, 2016

View reviewed changes

icereval and others added 16 commits January 11, 2017 13:41

Merge branch 'master' into develop-backup

5aa3fb9

Merge branch 'master' into develop-backup

7f4d97f

Merge branch 'master' into develop-backup

4f53f4f

Merge branch 'master' into develop-backup

e18cffe

Merge branch 'erinspace-hotfix/admin_permissions' into develop-backup

86c888a

Merge branch 'erinspace-hotfix/admin_permissions' into develop-backup

4e021f3

Merge branch 'master' into develop-backup

87d3fd9

Merge branch 'hotfix/scielo-agrixiv' into develop-backup

5618690

Merge branch 'hotfix/scielo-agrixiv' into develop-backup

c0415eb

Merge branch 'master' into develop-backup

fedc1d3

Merge branch 'master' into develop-backup

73480b5

Merge branch 'master' into develop-backup

a3edd3c

Merge branch 'master' into develop-backup

8748e3a

Merge branch 'master' into develop-backup

91cbba8

Merge branch 'master' into develop-backup

84e3b6e

Merge branch 'master' into develop-backup

0f95bbc

felliott requested changes Mar 14, 2017

View reviewed changes

icereval and others added 10 commits March 19, 2017 23:08

Merge branch 'master' into develop-backup

1989194

Merge branch 'master' into develop-backup

6483614

Merge branch 'master' into develop-backup

ebcbdd3

Merge branch 'master' into develop-backup

331b73f

copying github plugin as is

7c40bc7

changing github to gitlab

e927689

keep renaming github to gitlab

d117b5a

last renaming step

9741101

making tests green

1a5410d

use the right gitlab oauth uris

f576f73

danielneis and others added 18 commits March 28, 2017 16:09

fix last url

e7652cf

Fix gitlab addon models tests

0a8917e

refactoring

b3f80a5

minor fixes

5a181b1

fixing code style

e1c667d

fix more code style

e2359f8

fixes after mfraezz's review

16f8e65

changes to default branch

0bda68e

Fix TestNodeSettings

6255bd8

changes...

ba889c9

fix create repo

154ed47

fix host value to send to waterbutler

27cbcd7

Fix style issues

7bb5267

Rebase to develop-backup and some fixes based on review

66e1607

Fix outgoing OAuth request

48feb68

Use AddonIsRoot

6636d1d

Fix undefined item.data error

2580537

remove uneeded things for read-only + other fixes after review

963e9e0

danielneis force-pushed the feature/gitlab branch 2 times, most recently from 10aca21 to 963e9e0 Compare April 10, 2017 14:19

rafaeldelucena and others added 7 commits April 14, 2017 01:29

Use ref selected by the user

dc1002c

Add some missing things based on bitbucket addon

c10b890

Fixing zip/download and open urls + remove rename button

69bb4c9

Fix download and view on gitlab for repo on non-default branches

a99c170

Fix url to repository on public repo warning

bbe5684

Fix connect account duplicating

ae0a672

Fix connect account

0bfb021

mfraezz mentioned this pull request Jul 3, 2017

GitLab Addon [#OSF-8169] #7418

Merged

mfraezz closed this Jul 3, 2017



		# TODO: Test remaining CRUD methods
		# TODO: Test exception handling

		@@ -0,0 +1,10 @@
		{
		"gitlab_file_added" : "${user} added file ${path} to GitLab repo ${repo} in ${node}",

Conversation

danielneis commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Changes

Side effects

Roadmap

Uh oh!

mfraezz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felliott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mfraezz commented Jul 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

danielneis commented Jul 6, 2016 •

edited

Loading