[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI

[bugzilla at redhat.com]

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

Neal Gompa <ngompa13 at gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ngompa13 at gmail.com



--- Comment #2 from Neal Gompa <ngompa13 at gmail.com> ---
I've taken a quick look-over of the package, and I've noticed a few quirks.

* The worker subpackage has "Requires(post): os-autoinst >= 4" and "Requires:
os-autoinst < 5". This is rather strange, as usually semantic versioning
enforcement is in the same class of Requires (be it BuildRequires, Requires, or
Requires(*)). It's not usually mixed. Is there a good reason for this? The spec
didn't indicate anything obvious that would require it.

* Why aren't we running the tests in the %check section? We delete a test, but
then don't actually run any tests here.

* This is a bit of a style nitpick, but don't we usually use %{} format for
user-defined macros too? I see "%openqa_services" and "%openqa_worker_services"
instead of "%{openqa_services}" and "%{openqa_worker_services}"

* In the scriptlets, I don't see usage of macros for file paths that we use
elsewhere. This has the potential to break things if the macros were redefined
in the future. Please use them in the scriptlets. I believe they'll get
evaluated before being written to the package, so it shouldn't be a problem.

* If at all possible, could you split out the httpd configuration to a separate
subpackage? It might be possible in the future to get nginx or another
webserver supported for using with openQA, and it'd be nice if it wasn't
hardcoded from the get-go for httpd. You should be able to set up some kind of
virtual Provide to be required or use Requires (which would eventually turn
into a rich dependency) to handle this for ensuring openQA continued to work.

-- 
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