[Bug 1235305] Review Request: hitch - Network proxy that terminates TLS/SSL connections

[bugzilla at redhat.com]

https://bugzilla.redhat.com/show_bug.cgi?id=1235305



--- Comment #11 from Ingvar Hagelund <ingvar at linpro.no> ---
(In reply to Jeff Backus from comment #7)
> Hi folks,
> 
> I've done a formal review. Here are the highlights, with the formal review
> below:

Thanks, Jeff

New .src.rpm:
http://users.linpro.no/ingvar/varnish/hitch/hitch-1.0.0-0.3.3.beta3.fc22.src.rpm

New .spec: http://users.linpro.no/ingvar/varnish/hitch/hitch.spec

> * BR redhat-rpm-config isn't needed, however, as there is an on-going
> discussion about what should go in the minimum buildroot, I won't insist
> this is removed. Any reason this was added?

It used to be a requirement for hardened build, but that is probably long gone.
Fixed.

> * Even though BuildRequires allows multiple listings on one line, please
> only provide one. Specifically, line 25...

Fixed

> * Please add comments above %check section explaining why it is disabled by
> default and how to use it. Your explanation above is sufficient, just add it
> to the .spec file.

Fixed

> * Missed the -p when installing hitch.conf on line 98.

As the file is generated at build time, as was a bit unsure if this was
necessary. Fixed

> * Please remove the commented %setup macro in %prep.

Fixed

> * Please remove the commented commands in %build

Fixed 

> * While you're making changes, the description has "It's" but should be
> "Its"... :)

This was dumped from an upstream description, but still; "It is" -> "Its"??? 
I'll use "It is", to avoid any confusion.

> Overall, looks good. Please address the above and I'll approve it.

Great, thanks!

Ingvar

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component