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

Add basic gerrit support #183

Closed
wants to merge 7 commits into from

Conversation

Projects
None yet
3 participants
@nikitavbv
Copy link
Contributor

commented Dec 10, 2017

This introduces gerrit support requested in #19

README.md Outdated
@@ -99,7 +99,7 @@ You can create the request also by simply calling:

That command has a bit of automagic, it will:

1. lookup the namespace and project of the current branch (or at least on the `github`
1. lookup the namespace and project of the current branch (or at least on the `github`

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 11, 2017

Collaborator

could you move these unecessary modifications to a separate PR. I like nice clean small PRs that do one thing only.

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 11, 2017

Author Contributor

Okay. I think that change was done automatically by my editor and I didn't notice that. Totally agree - there is no need to have this unnecessary modification in this PR.

README.md Outdated
@@ -373,7 +373,7 @@ To use your own credentials, you can setup the following environment variables:
* [x] add support for gitbucket (cf [#142](https://github.com/guyzmo/git-repo/issues/142))
* [ ] add support for managing SSH keys (cf [#22](https://github.com/guyzmo/git-repo/issues/22))
* [ ] add support for issues (cf [#104](https://github.com/guyzmo/git-repo/issues/104))
* [ ] add support for gerrit (cf [#19](https://github.com/guyzmo/git-repo/issues/19))
* [x] add support for gerrit (cf [#19](https://github.com/guyzmo/git-repo/issues/19))

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 11, 2017

Collaborator

IMO your first initial implementation of just clone isnt enough to mark this as 'done'.

I think this is done when git-repo can do the equivalent of git review -d & git review with a single patch in the queue. (patchset support isnt conceptually supported by git-repo)

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 11, 2017

Owner

the minimal set of features to add a new service usually is: clone, fork, create, open, delete, add. In the context of gerrit, I'm not sure how all of them are relevant, but then not implementing those should be explicitly motivated.

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 11, 2017

Collaborator

fork isnt a thing, at all. People sometimes uses branches as an equivalent of forking.

As gerrit is a centrally managed fixed list of projects, delete and create would be super-user only operations. (very nice, but nobody would use them)

open would be a good addition to this first PR, IMO, as it requires that git-repo knows enough about the clone that it can fetch more metadata from the server.

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 11, 2017

Author Contributor

Okay. I will remove "done" mark as this PR introduces only basic functionality

@@ -215,7 +215,7 @@ def set_repo_slug(self, repo_slug, auto=False):

# This needs to be manually plucked because otherwise it'll be unset for some commands.
service = RepositoryService.get_service(None, self.target)
if len(namespace) > service._max_nested_namespaces:
if service._max_nested_namespaces and len(namespace) > service._max_nested_namespaces:

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 11, 2017

Collaborator

I dont think this is necessary. Just set max_nested_namespaces to be 99 if it truly is unlimited in gerrit (I doubt that; I am sure gerrit will break somewhere if the project list has 99 subtrees)

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 11, 2017

Owner

does it actually support project namespaces ?

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 11, 2017

Collaborator

Yup; Wikimedia has at least 5 levels of namespaces that I have seen. They might use more.

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 11, 2017

Author Contributor

Okay. max_nested_namespaces will be changed to 99

return namespace + '/' + repo

def is_repository_empty(self, project):
return False

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 11, 2017

Collaborator

it is impossible to have an empty repo in gerrit?

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 11, 2017

Author Contributor

It is possible to have an empty repo gerrit. This part is not implemented yet and I will check if there is a way to find out if repository is empty via API.

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

it's actually only used for clone, and to choose whether you want to start from scratch a project (git init in the project's directory) or actually pull from the remote. I'm not sure what's the process with gerrit, but I'm not sure it makes sense to create a new empty project on gerrit, does it?

if namespace:
return 'ssh://{}@{}:{}/{}/{}'.format(self._username, self.fqdn, self.port, namespace, repository)
else:
return 'ssh://{}@{}:{}/{}'.format(self._username, self.fqdn, self.port, repository)

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 11, 2017

Collaborator

how is this different from super().format_path(...) ?

needs unit tests please ;-)

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 11, 2017

Author Contributor

By the way, I think there is more optimal way to do this. I will change this and push updated version soon

@@ -215,7 +215,7 @@ def set_repo_slug(self, repo_slug, auto=False):

# This needs to be manually plucked because otherwise it'll be unset for some commands.
service = RepositoryService.get_service(None, self.target)
if len(namespace) > service._max_nested_namespaces:

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 11, 2017

Owner

you could send that as a separate PR to get it merged faster.


@classmethod
def get_auth_token(cls, login, password, prompt=None):
return password

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 11, 2017

Owner

doesn't gerrit support private token keys ? It'd be cool to avoid storing passwords in config files, but revokable tokens are 👌

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 11, 2017

Author Contributor

For rest API gerrit uses HTTP basic authentication with the HTTP password from the user’s account settings page. HTTP password is also used for cloning using http. So http password is something like private token here, not an actual account password. By the way, it can't be set by user. It is generated on separate settings page.
More info here: https://gerrit-review.googlesource.com/Documentation/rest-api.html#authentication

I expected that users will provide HTTP password each time they setup access to Gerrit.

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 15, 2017

Owner

ok, as long as the secret is revocable and not likely to be used elsewhere by the user, it's all good for me 👍

@guyzmo
Copy link
Owner

left a comment

globally it's a good start. But it lacks the tough part: choose a good and well maintained library that interfaces gerrit, and implement it.

About gerrit, I think the most important features would be request based ones.

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

Well, I checked for libraries interfacing gerrit yesterday. The problem is that there are no good and well maintained libraries for that.

The most functional seems to be this one: https://github.com/sonyxperiadev/pygerrit
However it is no longer maintained.

There is also a second version of this library available, which is currently in development: https://github.com/dpursehouse/pygerrit2

I am still not sure which of those libraries to use.
By the way, it is a question if we need to use any library at all - as there aren't much api calls we will need to make and it shouldn't be difficult to implement this without using any external libraries.

What is your opinion on this?

@jayvdb

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

Even though https://github.com/dpursehouse/pygerrit2 is experimental on Python 3, that should be OK. We can help them iron out any bugs. I am more concerned that they have dropped the SSH interface, and appear to be only using REST.

I suggest https://pypi.python.org/pypi/gerritlib ; openstack group will be good maintainers to work with.

https://github.com/tivaliy/python-gerritclient also looks like it going to be a good library.

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

Okay. Thank you! I will use one of those libraries and push updated code soon

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2017

Well I checked and tested all libraries. Here are the results.

https://github.com/tivaliy/python-gerritclient seems to offer more functionality. However there are problems with installing it with pip3: the library itself is compatible with python3, however it has cliff dependency (which is used only for cli by the way), which is not compatible with python3 and fails to install. Anyway, it is powerful and easy to use.

We may use https://pypi.python.org/pypi/gerritlib instead, however I checked its docs and source code and I am afraid it lacks functionality we need. It also doesn't work with REST API - it uses only SSH.

As for https://github.com/dpursehouse/pygerrit2 - it works fine. It has less functionality than python-gerritclient, however it easy to extend. For example, it doesn't have a function to get project info, however we can achieve that by using get method directly: restClient.get("/projects/namespace%2Frepo")

Personally, I liked python-gerritclient the most, however I am not sure if it is okay for us due to installation problems.

What's your opinion on this?

@jayvdb

This comment has been minimized.

Copy link
Collaborator

commented Dec 11, 2017

https://pypi.python.org/pypi/cliff and https://git.openstack.org/cgit/openstack/cliff/tree/setup.cfg#n15 says it supports Python 3.

Lets chat about this on gitter and maybe identity a bug which is missing from https://bugs.launchpad.net/python-cliff

@nikitavbv nikitavbv changed the title [WIP] Add gerrit support Add basic gerrit support Dec 12, 2017

:return: the full URI of the repository ready to use as remote
if namespace is not given, repository is expected to be of format
`<namespace>/<repository>`.
`<namespace>/<repository>` unless allow_no_namespace is set to True

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 12, 2017

Collaborator

period at the end.

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 12, 2017

Collaborator

also backticks for literal names

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2017

Oops. 3 commits. I will rebase it now

@@ -32,8 +32,8 @@

class ProgressBar(RemoteProgress): # pragma: no cover
'''Nice looking progress bar for long running commands'''
def setup(self, repo_name):
self.bar = Bar(message='Pulling from {}'.format(repo_name), suffix='')
def setup(self, repo_name, msg='Pulling from {}'):

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 14, 2017

Collaborator

instead of having custom messages being added by services, the constructor could take an 'action' argument which was an enum like PULL: 1; PUSH: 2 with PULL being the default to maintain backwards compatibility (so the rest of the code doesnt need to change). Then the two messages would be kept inside this class.

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 14, 2017

Author Contributor

Okay. I will do it that way

{
"http_interactions": [],
"recorded_with": "betamax/0.5.1"
}

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 14, 2017

Collaborator

Can we add a EOL of EOF here, or does that bugger up betamax?

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 14, 2017

Author Contributor

I am not sure about this. This file was generated automatically by betamax and other cassettes don't have EOL too.

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 14, 2017

Collaborator

Try it. Manually edit the file after generation. It may not write good JSON, but it will use the standard JSON loader, which can handle the proper EOF marker.

Worst than can happen is it doesn work and we raise a bug upstream.

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 14, 2017

Author Contributor

I added EOL and it works fine

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 15, 2017

Owner

well, don't worry about that, anyway those files will be overwritten at a later stage with actual data. About the proper JSON format, that will happen when we'll update to the latest betamax, so don't worry about that! 😉

Currently betamax is pinned to 0.5.1, because with a commit that will update only the tests, because it's painful to see all your tests rewritten because of an update of betamax, and not of your code.

@@ -863,3 +863,31 @@ def action_open(self, namespace, repository):
with Replace('subprocess.Popen', self.Popen):
self.service.open(user=namespace, repo=repository)

def action_review(self, namespace, repository, branch):

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 14, 2017

Author Contributor

I am not sure about this. Shall we keep it in helpers or it is better to move it to the gerrit test itself? Or may we try to use action_request_create instead?

@guyzmo

This comment has been minimized.

Copy link
Owner

commented Dec 15, 2017

Generally speaking about libraries, I chose to always use an interfacing library for the sake of only managing a python interface (so when it breaks, it breaks loudly and hard, and updating is handled using versions), and not an API interface.

And it's also for the greater good, as working on this project it made me get involved in python-gitlab, github3.py… and many other libraries for others to use.

@guyzmo

This comment has been minimized.

Copy link
Owner

commented Dec 15, 2017

about the review action, it'd be nice to think a bit on how to handle that use case in terms of CLI user experience. How do you see the interactions?

@@ -103,3 +103,20 @@ def request_create(self, onto_user, onto_repo, from_branch, onto_branch=None, ti
'project': '/'.join([onto_user, onto_repo]),
'remote': onto_branch
}

def request_fetch(self, user, repo, request, pull=False, force=False): #pragma: no cover

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 15, 2017

Owner

about #pragma: no cover, why are you excluding this function from coverage? As you've written tests for it, that can go :)

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 15, 2017

Author Contributor

Yes. It was placed by mistake. I will remove it now

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 15, 2017

about the review action, it'd be nice to think a bit on how to handle that use case in terms of CLI user experience. How do you see the interactions?

Well, this may need some discussion and improvement.
Right now it works like this: user creates commit and after that runs git [gerrit service name] request create. And it creates a new gerrit change. Unluckily, we don't show resulting change url (I am still thinking if we could do that - but due to some library limitations I don't see a clear way to do that). We also don't allow to set a custom topic or something like that.

I am still thinking on how we could improve that. I am open to any of your suggestions

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2017

Unluckily, we don't show resulting change url (I am still thinking if we could do that - but due to some library limitations I don't see a clear way to do that)

Good news: I found way to do that! Now we show resulting change urls.

@@ -272,24 +272,25 @@ def _convert_user_into_remote(self, username, exclude=['all']):
if self.fqdn in url and username == url.split('/')[-2].split(':')[-1]:
yield name

def format_path(self, repository, namespace=None, rw=False):
def format_path(self, repository, namespace=None, rw=False, allow_no_namespace=False):

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

I've had a thought about this, and the same way we can allow for a maximum of nested namespaces, we could setup a minimum as a class member which has the default of 2 for all services? And then here check that repo.count('/') >= self._min_nested_namespaces-1? I feel like that'd be a more generic way of handling this. But maybe I'm complicating things.

I guess the pros of the above option would be to keep the prototype simpler and factorize behaviour in a way homogeneous with _max_nested_namespaces. The cons would be to use a side effect to modify the method's behaviour.

@Phantom-42, @jayvdb : your opinions?

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 16, 2017

Author Contributor

I like this idea. Anyway, it would be interesting for me to hear @jayvdb opinion too

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 16, 2017

Author Contributor

And one more thing. I think we better set default _min_nested_namespaces to 1, and then check with repo.count('/') >= self._min_nested_namespaces. I think that makes a bit more sense and avoids having such confusing code as:

_max_nested_namespaces = 1
_min_nested_namespaces = 2

@guyzmo What do you think about that?

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

I totally agree with you on that! That makes sense!

In the above context I forgot that namespaces were excluding the project name 😉

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 16, 2017

Author Contributor

Okay. I changed the code to use _min_nested_namespaces now

@guyzmo
Copy link
Owner

left a comment

Possible improvements for the request_create's method output. I'd rather avoid the is_instance way, I got a preference over the third method, but I'm ok with the first method.

Opinions/Comments?

if 'url' in new_request:
log.info('available at: {url}'.format(**new_request))
if isinstance(new_request['url'], list):

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

ah! this is not very elegant… What value does adding checking the type of the URL here? I can see a few ways to get rid of that issue, either:

  • make the request_create method return a string "\n{}".format('\n'.join(…)), or
  • make the request_create method always return a list, and modify the other services to match that, or
  • make the do_request_create method delegate the way it displays stuff the same way we do with list methods (make that method a generator, where you first yield a format, then a header, then the values).

(I sorted the solution by order of laziness, the first one being the less work ☺)

Though, I believe the third option would be the most elegant, so that way you could control the exact message you want to output to the user to make git-repo more adapted to the gerrit way of things!

result['url'] = new_changes[0]
result['ref'] = result['url'].split('/')[-1]
else:
result['url'] = new_changes

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

following this review comment's you could do here as third option:

# header
yield "Created new review request of {local} onto {project}:{remote}".format(result['local'], result['project'], result['remote'])
# format
yield "with changeset {ref} available at {url}"
# values to apply on format
for change in new_changes
    yield {"url": change, "changeset": change.split('/')[-1]} # url + ref
@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2017

I like third method too. I just implemented it. Could you check please if everything is okay with it?

fmt = next(output_generator)
for item in output_generator:
log.info(fmt.format(**item))

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

you can simplify that using the print_iter function I wrote in the .tools module! Then all you got to do is:

print_iter(service.request_create(…))

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 16, 2017

Author Contributor

Okay. Changed that

@@ -272,24 +272,25 @@ def _convert_user_into_remote(self, username, exclude=['all']):
if self.fqdn in url and username == url.split('/')[-2].split(':')[-1]:
yield name

def format_path(self, repository, namespace=None, rw=False):
def format_path(self, repository, namespace=None, rw=False, allow_no_namespace=False):

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

I totally agree with you on that! That makes sense!

In the above context I forgot that namespaces were excluding the project name 😉

@@ -585,6 +614,9 @@ def request_fetch(self, user, repo, request, pull=False, force=False): #pragma:
def request_create(self, user, repo, from_branch, onto_branch, title, description=None, auto_slug=False):
raise NotImplementedError

def review(self):
raise NotImplementedError

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 16, 2017

Owner

so, what's your intention with that?

I assumed you were about to do some sort of git <target> request <project> review command.

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 16, 2017

Author Contributor

Sorry, I think I forgot to remove this function. Right, at first we wanted to have something like git <target> request <project> review, but then we decided to have git <target> request fetch <change-id> instead.

'''Push a repository
:param remote: git-remote instance
:param branch: name of the branch to push
'''

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 17, 2017

Collaborator

+:return:

@@ -862,4 +862,3 @@ def action_open(self, namespace, repository):
])
with Replace('subprocess.Popen', self.Popen):
self.service.open(user=namespace, repo=repository)

This comment has been minimized.

Copy link
@jayvdb

jayvdb Dec 17, 2017

Collaborator

you dont need to modify this line.

@guyzmo
Copy link
Owner

left a comment

I've actually tested your code by running it. So thank you a lot for your work so far, now we're getting into the real fun! 😁

  • first change it's missing, is to add in README.md that one need to connect to the http password page to get the token.

  • second thing, is to make request fetch homogeneous with the way it works with the other services, i.e. act like a git fetch not like a git pull.

  • third thing, is to make action_ stuff in the helpers module so the tests behaviors are managed from a single place. request_fetch should be handled with the existing action_request_fetch (and then you'll fix the tests for the other services), though for the request_create, there's some thinking to do (cf my comments)

  • another change I'm asking you is another thing with print_iter (cf matching comment).

  • I've also noticed there's a regression with git <target> add without name, which used to be "guessing" the repo name (based on existing remotes URLs), and does not anymore. And I'd like to see that work with gerrit as well.
    It might be a race condition that exists because of the fix in kwargparse (I'm not saying your fix is wrong, just that we might need yet another fix 😉).

Thank you again for all ♥

log.info('available at: {url}'.format(**new_request))

log.info(next(output_generator))
print_iter(output_generator)

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 19, 2017

Owner

You should not output the first line using log.info, as the first line output by print_iter is dependent on whether you're in interactive mode or in tty mode (so that the header is skipped when piping the output to some other command). cf next comment for what I'd suggest to do:

ref=request.id
)
yield 'available at {}'
yield request.links['html']['href']

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 19, 2017

Owner

here's a way:

yield "{}" # format
yield 'Successfully created request of `{local}` onto `{project}`: {remote}, with id `{ref}`'.format( ... )
yield 'available at {}'.format(request.links['html']['href'])

then we're a bit more script friendly ;)

self.fetch(remote, request)
self.repository.git.checkout('FETCH_HEAD')

return 'FETCH_HEAD'

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 19, 2017

Owner

ok, two questions here:

  • why are you fetching the request in detached state, and not as a full ref?
  • why are you checking it out right away?

The idea of request fetch command, is to mimic the concept of git fetch which, unlike a pull gets a ref from the remote repository, but does not change the state of your workspace. That way you can fetch a request, with a dirty workspace. And when you checkout that ref, only then you create a mess (or not).

As it's how the others are implemented, I'd prefer to see gerrit work the same way to keep things homogeneous. Also, it'll make things easier for the tests, so that you won't have to overload the action_fetch method.

ref=request.number
)
yield 'available at {}'
yield request.html_url

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 19, 2017

Owner

There you've changed the way request_create works across the tool, but as you're overloading action_create in gerrit, you did not update the overloaded method, and now all request_create tests fail! ☹

def get_requests_session(self):
return self.service.session

def action_request_create(self, namespace, repository, branch):

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 19, 2017

Owner

because action_request_create is quite different from the others, maybe it's a good thing to overload it. The only thing that bugs me is that we lose the factorisation it offers in helpers.py, so that with a change alike the one you made going from return to yield, you change it once for every service, and once for all the tests.

Maybe a good idea could be to simply not overload action_request_create in the service, but instead move that method to helpers.py, name it action_request_create_by_push and leave it alongside action_request_create.

])
with self.recorder.use_cassette(self._make_cassette_name()):
self.service.connect()
self.service.request_fetch(namespace, repository, request)

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 19, 2017

Owner

I see no good reason to overload action_request_fetch, especially if you follow the way it's working for the other services (fetching the request as a new ref, give the name of the ref, but not switch to it).

If you really like the behaviour of fetch + checkout, then we could consider creating a request pull option 😉

@guyzmo
Copy link
Owner

left a comment

A couple of changes that are cosmetics. But I did not have to do a deeper check of the patchset. I'll try to take some time tonight for that, I'm at a conference today

if 'url' in new_request:
log.info('available at: {url}'.format(**new_request))

print_iter(output_generator)

This comment has been minimized.

Copy link
@guyzmo

guyzmo Dec 21, 2017

Owner

you can remove the useless intermediate variable (it does not add much to readability):

print_iter(service.request_create(
        self.repo_name,
        self.local_branch,
        self.remote_branch,
        self.title,
        self.message,
        self._auto_slug,
        request_edition
    )
)

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Dec 22, 2017

Author Contributor

Done!

@guyzmo

This comment has been minimized.

Copy link
Owner

commented Jan 17, 2018

damn! ☹ @Phantom-42 it's been a month since last time I checked around here, and it feels like it was yesterday… I do not forget about you, though, but I'm still totally snowed under at work ☹

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Jan 17, 2018

No worries! I fully understand you. Just notify when you will have some free time and we will continue!
By the way, I really like this tool and I use it on regular basis now. I would be happy to work on more features and pull requests in the future! :)

@guyzmo
Copy link
Owner

left a comment

it sounds we're definitely getting there… I had some trouble testing it, but it sounds more like issues with me being not familiar with the working of gerrithub than with your code.

Just a few comments to smooth things on, but I guess next update I'll be approving and merging it 😉

else:
change_id = request.split('/')[3]

remote = self.repository.remote(self.name)

This comment has been minimized.

Copy link
@guyzmo

guyzmo Feb 3, 2018

Owner

what if the gerrit remote does not exists?

maybe handling an error and issue a warning guiding the user to do something like: git-repo gerrit add <slug> (or doing it directly) could be a good idea ;)

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Feb 4, 2018

Author Contributor

Done. Could you check please and tell if message is okay. If you have some ideas, we may change it to something better if needed.

remote_ref='refs/changes/89/392089/1',
local_ref='requests/gerrithub/392089')

def test_04_fetch_patchset__full(self):

This comment has been minimized.

Copy link
@guyzmo

guyzmo Feb 3, 2018

Owner

typo: patchset

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Feb 4, 2018

Author Contributor

Sorry, but I think there is no typo here

remote_ref='refs/changes/08/391808/2',
local_ref='requests/gerrithub/391808')

def test_05_fetch_patchet__change_id(self):

This comment has been minimized.

Copy link
@guyzmo

guyzmo Feb 3, 2018

Owner

typo: patchset

remote_ref='refs/changes/89/392089/2',
local_ref='requests/gerrithub/392089')

def test_06_list_patchets(self):

This comment has been minimized.

Copy link
@guyzmo

guyzmo Feb 3, 2018

Owner

typo: patchsets

def request_create(self, onto_user, onto_repo, from_branch, onto_branch=None, title=None, description=None, auto_slug=False, edit=None):
from_branch = from_branch or self.repository.active_branch.name
onto_branch = onto_branch or 'HEAD:refs/for/' + from_branch
try:

This comment has been minimized.

Copy link
@nikitavbv

nikitavbv Feb 4, 2018

Author Contributor

I think we need this message here as well, right?

@nikitavbv

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2018

I had some trouble testing it, but it sounds more like issues with me being not familiar with the working of gerrithub than with your code.

If needed, please tell and I will be happy to help you with gerrithub, so you can test this better! Thank you for reviewing!

@guyzmo

guyzmo approved these changes Feb 4, 2018

@guyzmo guyzmo closed this Feb 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.