Full review (do not merge)

theforeman / smart_proxy_abrt

Automatic Bug Reporting Tool plugin for Foreman's smart-proxy

GNU General Public License v3.0

0 stars 5 forks source link

Full review (do not merge) #1

Closed mmilata closed 10 years ago

lzap commented 10 years ago

Note for ourselves - don't forget to squash before merging.

First question to broader audience - license GNU v3 should be compatible with Foreman's v2 right?

lzap commented 10 years ago

Looks good to me. I will wait for the packaging work and all the dependencies so I can test it on production instance.

mmilata commented 10 years ago

Thanks for the review! Isn't Foreman (as well as smart-proxy) licensed under GPLv3?

domcleal commented 10 years ago

Foreman's GPLv3+, yes, so plugins should be too.

mmilata commented 10 years ago

@witlessbird thank you for the review. I tried to address all the issues except

usage of WEBrick::HTTPRequest,
the plugin being accessible through HTTP.

Please see the respective inline comments.

dmitri-d commented 10 years ago

Looks pretty good, just some pretty minor stuff that's left...

mmilata commented 10 years ago

I have pushed some new commits that should address all of the issues since the last batch of commits.

dmitri-d commented 10 years ago

looks good to me!

lzap commented 10 years ago

Ok for the record - I did initial review via e-mail already before this PR was raised.

I think this is good to go, until @witlessbird has some open things we are ready to squash, package, test and merge. Ping me once packages are ready for testing. Good work! Thanks.

dmitri-d commented 10 years ago

looks good to me.

mmilata commented 10 years ago

@lzap @witlessbird Thank you for your time.

I've released a new version and created pull request against foreman-packaging: https://github.com/theforeman/foreman-packaging/pull/359 Please let me know when I should do that with the Foreman plugin as well.

lzap commented 10 years ago

I can confirm ABRT plugin and proxy plugin work on RHEL 7.0. I haven't tested RHEL 6 because there is no support in this version of the plugin. Upgraded abrt packages and new version of rubygem-ffi (EPEL6 is too old) would allow to bring the support, but let's do this in a separate effort.

If there are no objections, I am merging both plugins. Thank you @mmilata for outstanding work on this.

lzap commented 10 years ago

I guess this can be closed, its not meant to be merged the same way as the https://github.com/theforeman/foreman_abrt/pull/1 PR

Thanks!