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 missing AbandonChange, RebaseChange, RestoreChange and RevertChange #41

Merged
merged 8 commits into from
Sep 14, 2017

Conversation

opalmer
Copy link
Contributor

@opalmer opalmer commented Sep 14, 2017

Discovered AbandonChange was missing when I was working on Change.Abandon() in gerrittest. Figured I'd implement it in go-gerrit and the other missing functions too since they're nearly identical.

@opalmer opalmer added this to the 0.5.1 milestone Sep 14, 2017
@opalmer opalmer self-assigned this Sep 14, 2017
@codecov-io
Copy link

codecov-io commented Sep 14, 2017

Codecov Report

Merging #41 into master will increase coverage by 0.97%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   17.94%   18.92%   +0.97%     
==========================================
  Files          21       21              
  Lines        1750     1760      +10     
==========================================
+ Hits          314      333      +19     
+ Misses       1403     1393      -10     
- Partials       33       34       +1
Impacted Files Coverage Δ
changes.go 16.14% <100%> (+11.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a2e6113...6c23053. Read the comment docs.

Copy link
Owner

@andygrunwald andygrunwald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Unit tests would be a nice addition here. Up to you, feel free to merge if you want.

@opalmer
Copy link
Contributor Author

opalmer commented Sep 14, 2017

@andygrunwald, done!

Most of changes.go is missing test coverage including the existing SubmitChange function which is why I didn't add tests initially. But, the new functions are nearly identical so they're simple to test (which is also why I consolidated the code in 75d549b so we're not repeating ourselves unnecessarily.

It would be better if we had a real instance of Gerrit to test against but until gerrittest is finished using the httptest works well for these kinds of changes.

@opalmer opalmer merged commit 9cd8431 into master Sep 14, 2017
@opalmer opalmer deleted the add-missing-endpoints branch September 14, 2017 12:23
@andygrunwald
Copy link
Owner

andygrunwald commented Sep 14, 2017

@opalmer Thanks for your effort.

Most of changes.go is missing test coverage

True, i think because i was lazy. Sorry :(
But as you wrote, for new functionality it make sense. I am not striving / running for a 100% coverage, but a few tests for the start might be a good idea to motivate more people to go for tests.

It would be better if we had a real instance of Gerrit to test against

Agree. And you are working somehow on it. So tests which is covered twice, doesn't hurt :)

Thank you! I need to serve you a 🍺 or ☕️ or 🍸 some day

@opalmer
Copy link
Contributor Author

opalmer commented Sep 14, 2017

True, i think because i was lazy. Sorry :(

No worries and I'm not trying to blame anyone :). Most parts of go-gerrit are being tested or used in other ways so the lack of coverage is not necessarily an indication that there's a real problem. I brought it up mainly because it's something that could be worked on is all.

Agree. And you are working somehow on it. So tests which is covered twice, doesn't hurt :)

Agreed. gerrittest should solve the 'how do we run and test against a real instance of gerrit'. Matter of fact, that part is already done:

The bits I'm finishing up in gerrittest right now are for another project that I'm hoping to open source soon-ish. It's basically a bot for Gerrit written in Go and since it's using go-gerrit under the hood I wanted to write gerrittest so I could do integration-like testing while at the same time solving #11.

Thank you! I need to serve you a 🍺 or ☕ or 🍸 some day

Well if you're ever near the Atlanta area let me know, I owe you a drink too for writing go-gerrit in the first place 😄

@andygrunwald
Copy link
Owner

Sounds good! What is this gerritbot doing?

I had / started two projects in the past related to bots and gerrit. See https://github.com/andygrunwald/gotrap and https://github.com/andygrunwald/watson.
Not my best work, but you know, we learn with every line of code ;)

@opalmer
Copy link
Contributor Author

opalmer commented Sep 14, 2017

Right now what it can do is:

  • Reapply Code-Review/Verified +1 labels if certain conditions are met (rebasing a patch set with no code change or changing the commit message for example).
  • Send direct email to specific individuals (ping <user2>, <user2>).
  • Send direct emails to everyone who's CCed on a code review (ping).
  • Perform post-merge tasks (merge master -> other branch, build and publish documentation, etc) and post failures directly back to the code review so someone can take action.

In terms of how it works today:

  • The bot reads events from the events-log plugin.
  • Handlers register themselves to listen for specific kinds of events.
  • The bot passes the event struct into the handler and provides an instance of go-gerrit as well as few other convenience functions.

It's already fairly close to a framework of sorts for interacting with Gerrit but I'm planning to modify the existing bot a bit before making it open source. Functionally it will be the same but I'm going to use go-plugin to connect parts of the bot together so it functions more like this:

Gerrit > Events [events-log plugin or gerrit stream-events] > input plugin > gerritbot [core] > handler plugin > gerritbot > output plugins [email, post comment, etc]

I think output plugins will be fairly limited to common operations because a handler plugin could always do that work itself. The input plugins could do some caching/persistence but I'm trying to keep the core of the bot and the handler plugins event based if that makes any sense.

@andygrunwald
Copy link
Owner

Sounds great!
When this bot-(framework) is done, you can add this into the chapter of go-gerrit as an "Who implemented this lib?" thingy.

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

Successfully merging this pull request may close these issues.

3 participants