https://bugzilla.redhat.com/show_bug.cgi?id=1304882 Neal Gompa <ngompa13@xxxxxxxxx> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ngompa13@xxxxxxxxx --- Comment #2 from Neal Gompa <ngompa13@xxxxxxxxx> --- 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 _______________________________________________ package-review mailing list package-review@xxxxxxxxxxxxxxxxxxxxxxx https://admin.fedoraproject.org/mailman/listinfo/package-review