Take a crack at issue #174 #175

AmandaCameron · 2017-08-05T12:34:19Z

This is a WIP attempt at making the tool agnostic to subgroups as presented in #174

AmandaCameron · 2017-08-05T13:01:30Z

Worth noting, this PR also fixes #172 because the reason I needed the subgroup support was to file a bunch of MRs at once for a wide-sweeping change.

AmandaCameron · 2017-08-05T13:12:07Z

The tests are still failing, but it's because of this test here: https://github.com/guyzmo/git-repo/blob/devel/tests/integration/test_main.py#L120-L122 not sure what we should do about that (disable the test / set it up so it'll work )

guyzmo · 2017-08-06T11:08:24Z

Well, this test was implemented to make sure the repo_slug invariant is right, and that invariant was, up until your patch, having one and only one slash in it.

Now, your patch is changing the invariant, thus the test shall change. We'll also need to check the side effects of that patch, i.e. checking what kind of errors will happen with other services when asking for multiple slash slugs that would then be invalid. (I think there's a sort of grouping implementing in bitbucket, though).

guyzmo · 2017-08-06T11:09:39Z

git_repo/repo.py

-            if len(overflow) != 0:
-                raise ArgumentError('Too many slashes.'
-                                    'Format of the parameter is <user>/<repo> or <repo>.')
+            #self.namespace, self.repo_name, *overflow = self.repo_slug.split('/')


as a todo note: don't forget to remove that commented line when you'll be done 😉

guyzmo · 2017-08-06T11:11:53Z

git_repo/repo.py

-            self.user_name, self.repo_name, *overflow = self.repo_slug.split('/')
-            if len(overflow) != 0:
-                raise ArgumentError('Too many slashes.'
-                                    'Format of the parameter is <user>/<repo> or <repo>.')


maybe, this test could stay with a check on some member that would be: if self.service._single_member_namespace: raise ArgumentError(…) So that services (like github? gitea?…) that don't implement namespaces >1 level would still raise that error.

guyzmo · 2017-08-06T11:12:40Z

git_repo/repo.py

-                if not self.user_name:
+                if not self.namespace:
                    raise ArgumentError('Cannot clone repository, '
                                        'you shall provide either a <user>/<repo> parameter '


don't forget to change <user>/<repo> as well in the __doc__ at the top of the file (the --help) and on other occurences like here! 😉

guyzmo · 2017-08-06T11:14:15Z

git_repo/services/ext/gitlab.py


            if not from_project:
-                raise ResourceNotFoundError('Could not find project `{}/{}`!'.format(from_user, from_repo))
+                raise ResourceNotFoundError('Could not find project `{}`!'.format(from_reposlug))


guyzmo · 2017-08-06T11:14:38Z

git_repo/services/service.py

        if '://' in url:
-            *_, user, name = url.split('/')
-            return '/'.join([user, name])
+            # *_, user, name = url.split('/')


todo: remove comment 😉

guyzmo

So far so good! Only a few minor comments inline, just need to have proper test cases (for repo.py, and for each service). I introduce you about the tests system on IRC.

guyzmo

simple change for nested namespace limiting

guyzmo · 2017-08-08T21:21:22Z

git_repo/services/ext/gitlab.py

 class GitlabService(RepositoryService):
    fqdn = 'gitlab.com'

+    _supports_nested_namespaces = True


here I would change that to max_nested_namespaces = 21

as gitlab supports 21 subgroups maximum (if I remember right), so a good idea would be to set it up to that number!

guyzmo · 2017-08-08T21:22:14Z

git_repo/repo.py

-            #self.namespace, self.repo_name, *overflow = self.repo_slug.split('/')
            *namespace, self.repo_name = self.repo_slug.split('/')
            self.namespace = '/'.join(namespace)
+            if len(namespace) > 1 and not self.service._supports_nested_namespaces:


here you could do something like if len(namespace) >= self.service.max_nested_namespaces

guyzmo · 2017-08-08T21:23:08Z

git_repo/services/service.py

            'server-cert'
            ]

+    _supports_nested_namespaces = False


and here max_nested_namespaces would be 1 as default!

guyzmo · 2017-08-08T21:26:48Z

tests/integration/test_gitlab.py

+    def test_04_clone__subgroup(self):
+        self.action_clone(namespace='git-repo-test/subgroup',
+                          repository='test-repo')
+


here you could add another test that would be:

def test_04_clone__subgroup_too_many(self): self.action_clone(namespace='git-repo-test/a/b/c/d/e/f/g/h/i/j/k/l/m/n/o/p/q/r/s/t/u/v/w/x/y/z', repository='test-repo')

guyzmo · 2017-08-08T21:32:18Z

tests/integration/test_main.py

-        assert None is did_clone
+    # def test_clone__too_many_slashes(self):
+    #     did_clone = self.main_clone('guyzmo/git/repo', 2)
+    #     assert None is did_clone


ok, I understand that this one is less obvious, and I'll make it a todo for me. So if you don't feel like it, leave it commented out, it's on me 😉

Basically, the way I'd implement it would be to add a new optional parameter to self.main_clone() that specifies the number of allowed backslashes, so that the test would be:

def test_clone__not_too_many_slashes(self): did_clone = self.main_clone('guyzmo/git/repo', allowed_ns=3, 0) def test_clone__too_many_slashes(self): did_clone = self.main_clone('guyzmo/git/repo', allowed_ns=1, 2)

where the main_clone helper would change the allowed value of RepositoryService's mockup before calling the do_clone() function.

guyzmo

overall your tests look fine, and the errors you're getting are likely to be hicups with betamax configuration (I'm still struggling to find the right way to have it configured so that it generates the cassettes fine for anybody else than me).

Beyond my comments above about the number of allowed nested namespaces, your PR looks pretty much fine to me.

I'll need to find some time ASAP (likely this week end or early next week) to rebuild the cassettes from your tests and check that it's all working smoothly and nicely.

Until then, please use your own branch for what you need to double check that it's just working beautifully and you've not missed anything! (consider that the poor person's staging 😉).

Thank you again a lot for your contribution ♥👍🍻

guyzmo

lovely ♥ 🙌

AmandaCameron · 2017-08-12T01:57:20Z

I'm seeing a lot of AttributeErrors inside the various tests, and I'm having a hard time understanding where they're coming from, but most of the test failures are around that. It seems like some piece of the magic is breaking down, but I'm not familiar enough with the whole picture to understand how / why.

…the possibility of it being unset.

guyzmo · 2017-11-18T19:43:47Z

@timfonic I'm sorry for the time to merge and answer, but cf the readme file I'm getting pretty busy, and I don't have enough time to give alone all the care this project needs.

AmandaCameron added 2 commits August 4, 2017 18:08

Pass 1 at allowing slashes in the 'user_name' part.

804e55c

Apply feedback given in IRC.

3bcf6e4

Resolve circular dep loop by reversing where EXTRACT_URL_RE comes from.

4122bf4

guyzmo reviewed Aug 6, 2017

View reviewed changes

guyzmo requested changes Aug 6, 2017

View reviewed changes

AmandaCameron added 2 commits August 6, 2017 14:38

Apply some of the feedback from the PR.

58cdb32

Implement a bunch of tests, disable the too_many_slashes in test_main

3c31657

guyzmo requested changes Aug 8, 2017

View reviewed changes

guyzmo reviewed Aug 8, 2017

View reviewed changes

_supports_nested_namespaces -> _max_nested_namespaces

cde1d02

guyzmo approved these changes Aug 11, 2017

View reviewed changes

Manually grab the service inside set_repo_slug to prevent a loop and …

879f82e

…the possibility of it being unset.

guyzmo closed this Nov 18, 2017

Take a crack at issue #174 #175

Take a crack at issue #174 #175

Uh oh!

Conversation

AmandaCameron commented Aug 5, 2017

Uh oh!

AmandaCameron commented Aug 5, 2017

Uh oh!

AmandaCameron commented Aug 5, 2017

Uh oh!

guyzmo commented Aug 6, 2017

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!

guyzmo left a comment

Choose a reason for hiding this comment

Uh oh!

guyzmo 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!

guyzmo left a comment

Choose a reason for hiding this comment

Uh oh!

guyzmo left a comment

Choose a reason for hiding this comment

Uh oh!

AmandaCameron commented Aug 12, 2017

Uh oh!

guyzmo commented Nov 18, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants