Add support for querying the events-log plugin #10

opalmer · 2016-07-30T16:21:07Z

This PR adds support for querying information about Gerrit events via the optional events-log plugin. The structures this PR adds could also be used when parsing output from the built-in gerrit stream-events command over ssh.

…ent.Do

coveralls · 2016-07-30T16:22:36Z

Coverage decreased (-0.1%) to 9.087% when pulling 8ec2f65 on opalmer:events_log into 9dc3026 on andygrunwald:master.

opalmer · 2016-07-30T16:26:58Z

@andygrunwald PTAL. It was not clear to me if there was anything for or against providing a function to parse the output of an optional plugin so I thought I'd open up a PR to discuss it. Even without GetEvents() it would seem that providing the structures of a Gerrit event would be pretty useful since they could be used for parsing the output of the stream-events command.

Also, how do you think this should be tested? Most of the tests today talk to an external server which for this code won't work since it requires auth. Have you thought about having the tests spin up an http server that can provide responses to requests?

andygrunwald · 2016-07-30T17:44:21Z

Thank you @opalmer.
Is this PR for the support of this event-log plugin?

I don`t have a problem with adding support for plugins. The plugins needs to "kind of" popular, but as far as i can see / know this applies to event-log.
And as you said the structs are able to reuse. So this is fine :)

Related to testing:
Yes, you can bootstrap an own http server for tests. I did this in a different project. For this is have a setup() and tearDown() func. Those will bootstrap the http server. And here is a test how to use this: See TestProjectService_GetList. If you have a look at different tests in the go-jira you will encounter a pattern there.
This can be applied to this client library as well.
Do you plan to write tests for this PR / would you be so nice to write a few unit tests?

And it would be cool if you can add a note into the README (e.g. Features section) that we have support for this plugin as well.

opalmer · 2016-07-31T13:38:22Z

Is this PR for the support of this event-log plugin?

That's the one.

I don`t have a problem with adding support for plugins. The plugins needs to "kind of" popular, but as far as i can see / know this applies to event-log. And as you said the structs are able to reuse. So this is fine :)

Awesome, thanks! I'm happy to add some notes to the README alone those lines if you think that would be appropriate for future contributors.

Do you plan to write tests for this PR / would you be so nice to write a few unit tests?

Sure happy to though it may be a bit before I can get it done (have some other things I'm working on at the moment).

And it would be cool if you can add a note into the README (e.g. Features section) that we have support for this plugin as well.

Will do.

coveralls · 2016-08-01T18:55:13Z

Coverage increased (+1.5%) to 10.698% when pulling 0c4174c on opalmer:events_log into 9dc3026 on andygrunwald:master.

opalmer · 2016-08-01T18:56:28Z

@andygrunwald PTAL. Added tests, updated the README and fixed a couple of bugs.

On a side note, I'll probably be opening another PR shortly to update the imports in the tests from github.com/andygrunwald/go-gerrit to .. Without this I had to do some $GOPATH manipulation in order for the tests to use my forked repository.

EDIT
After looking around one answer for forked repository testing seems to be this: http://blog.campoy.cat/2014/03/github-and-go-forking-pull-requests-and.html

Using . as an import path seems less standard than the setup above. But, maybe the thing to do is to change the package name of the tests from gerrit_test to gerrit instead? This would mirror the setup used by your go-jira repository as well as many other Go projects. It would also fix testing for forks without having to go through the setup from the link above. Thoughts?

andygrunwald · 2016-08-02T06:31:27Z

Thank you. I merged all changes.

Related to the tests:
I am fine with renaming the test package to gerrit_test.
This would have the big benefit that we are only able to test the client api and no internal state (this would bring us the behaviour to create faulty tests).
Go for it :)

opalmer · 2016-08-02T15:55:46Z

The current name of the test package is gerrit_test though and I was
proposing that be changed to gerrit. If you prefer to keep the existing
name, which I don't object to for the reason you mentioned, are you ok with
an update to the imports so they're relative instead (.)?

On Aug 2, 2016 2:31 AM, "Andy Grunwald" notifications@github.com wrote:

Thank you. I merged all changes.

Related to the tests:
I am fine with renaming the test package to gerrit_test.
This would have the big benefit that we are only able to test the client
api and no internal state (this would bring us the behaviour to create
faulty tests).
Go for it :)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#10 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPkjaXQJ7rWJwJSH0ioK6urk6S0NTedks5qbuQ_gaJpZM4JY1xK
.

andygrunwald · 2016-08-03T14:36:25Z

@opalmer I don't like the change with the import path. I understand your problem related to the fork. Would it be an option for you to
a) apply the git remotes way like in the blog post you mentioned
or (maybe better)
b) be a contributor with write access to this project? Then i would add you to this.

opalmer · 2016-08-09T15:20:47Z

@andygrunwald sorry for the delay, I've been traveling. Totally understand wanting to keep the import path as is so no argument there.

Options B would be nice if you're ok with it 😄 . I'll still open PRs of course but having write access would help with development. Another added benefit is if you're ever away I can sub in for bug fixes and code reviews. On a related side note, it's probably a good idea if I try out option A anyway and add it to the README once I'm done. The setup is not all that complicated but a little documentation around the procedure, and why it's necessary, could help a future contributor.

andygrunwald · 2016-08-09T15:42:40Z

No worry.
I`ve added you as an collaborator. You just need to accept the invite. Welcome :)
It would be good to open PRs for changes that we or other people are able to review them. Sometimes magic things can happen :)

Looking forward to your changes.

dmitshur · 2017-05-16T20:02:50Z

gerrit.go

+		if index > -1 {
+			// +1 to catch the \n as well
+			body = body[(index + 1):]
+		}
 	}


This code can be significantly simplified without changing behavior.

If body has ")]}'\n" prefix, there's no need to use bytes.IndexByte to find the first '\n' character, it's known to be at index 4 because bytes.HasPrefix(body, []byte(")]}'\n")) was true.

Also, since this is called often, it's probably a good idea to factor out []byte(")]}'\n") into a single package scope variable, instead of potentially allocating once per RemoveMagicPrefixLine call.

func RemoveMagicPrefixLine(body []byte) []byte { if bytes.HasPrefix(body, magicPrefix) { return body[5:] } return body } var magicPrefix = []byte(")]}'\n")

@shurcooL, no issues here if you want to go ahead and make the changes. I'd do it myself but I don't currently have my local host setup to make the change and test it.

@opalmer I already made a PR that applies this suggestion, see #36.

Ah my bad sorry about that. Reviewed, thanks!

Thanks both of you @opalmer and @shurcooL

opalmer added 7 commits July 24, 2016 21:23

initial commit of EventsLogService

0aea9fc

only strip the magic prefix if it's present

1f730e7

handle response in GetEvents() rather than passing it into events.cli…

5d8fb6b

…ent.Do

some minor cleanups to var names

13c9442

adding test for new behavior in RemoveMagicPrefixLine

cb50e8d

fix check for From/To

eca621f

fixing lint issues

8ec2f65

opalmer added 6 commits August 1, 2016 11:05

fix variable name which is shadowing package

eff38ce

adding note in README about events-log support

d6d7c7f

add note to the FAQ about plugin support

b48c378

url -> requestURL so we don't shadow the url package name

a0e5b42

add url arguments as part of the raw query

968de84

adding tests

0c4174c

andygrunwald merged commit 0c4174c into andygrunwald:master Aug 2, 2016

opalmer deleted the events_log branch August 2, 2016 22:14

opalmer modified the milestone: 0.1.0 Oct 8, 2016

opalmer self-assigned this Oct 8, 2016

opalmer added the enhancement label Oct 8, 2016

dmitshur reviewed May 16, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for querying the events-log plugin #10

Add support for querying the events-log plugin #10

opalmer commented Jul 30, 2016

coveralls commented Jul 30, 2016 •

edited

Loading

opalmer commented Jul 30, 2016

andygrunwald commented Jul 30, 2016

opalmer commented Jul 31, 2016

coveralls commented Aug 1, 2016 •

edited

Loading

opalmer commented Aug 1, 2016 •

edited

Loading

andygrunwald commented Aug 2, 2016

opalmer commented Aug 2, 2016

andygrunwald commented Aug 3, 2016 •

edited

Loading

opalmer commented Aug 9, 2016 •

edited

Loading

andygrunwald commented Aug 9, 2016

dmitshur May 16, 2017 •

edited

Loading

opalmer May 17, 2017

dmitshur May 17, 2017

opalmer May 17, 2017

andygrunwald May 17, 2017

Add support for querying the events-log plugin #10

Add support for querying the events-log plugin #10

Conversation

opalmer commented Jul 30, 2016

coveralls commented Jul 30, 2016 • edited Loading

opalmer commented Jul 30, 2016

andygrunwald commented Jul 30, 2016

opalmer commented Jul 31, 2016

coveralls commented Aug 1, 2016 • edited Loading

opalmer commented Aug 1, 2016 • edited Loading

andygrunwald commented Aug 2, 2016

opalmer commented Aug 2, 2016

andygrunwald commented Aug 3, 2016 • edited Loading

opalmer commented Aug 9, 2016 • edited Loading

andygrunwald commented Aug 9, 2016

dmitshur May 16, 2017 • edited Loading

Choose a reason for hiding this comment

opalmer May 17, 2017

Choose a reason for hiding this comment

dmitshur May 17, 2017

Choose a reason for hiding this comment

opalmer May 17, 2017

Choose a reason for hiding this comment

andygrunwald May 17, 2017

Choose a reason for hiding this comment

coveralls commented Jul 30, 2016 •

edited

Loading

coveralls commented Aug 1, 2016 •

edited

Loading

opalmer commented Aug 1, 2016 •

edited

Loading

andygrunwald commented Aug 3, 2016 •

edited

Loading

opalmer commented Aug 9, 2016 •

edited

Loading

dmitshur May 16, 2017 •

edited

Loading