On Mon, 28 May 2012 15:02:21 +0200, Mo Morsi <mmorsi(a)redhat.com> wrote:
Hrm, one thing that I don't like about these pull requests is
makes it tricky to comment of specific bits of code in the review. With
the send-patches-to-list approach, I can comment inline on specifics.
Agreed. I mainly did it this way because the changeset was so large,
that (1) sending that much diff in email is a bit excessive, imo, and
(2) I was seeing some issues where the patches were getting mangled and
would not apply properly. Possibly due to file renames and the fuzzy
logic surrounding them. I didn't look too much into it.
Regardless, a few notes:
1 - in the original patchset you mentioned that there was a prerequisite
patch needed ("Various test and rake fixes for F17 compatibility") to
build the rpms. I couldn't find this on the list (did I miss it?)
2 - I noticed in this patchset some modules were placed under the
'aeolus' namespace (eg the rails and rhevm config) while some modules
were placed at the top level (eg the apache, ntp, openssl, postgres
config). Was there a reason for this distinction?
I intentionally didn't move any code across modules. That sort of
reorganization is definitely needed, but I didn't want to creep too far
from the goal of "get these modules to run under 2.7". I think selinux
was the only other place this was applicable, for future reference.
3 - for the rails db manifests, I'd rather switch the ordering so
'db' is the folder which contains puppet modules for 'drop',
'create', 'migrate'. I'd be fine w/ including this change in a
Agreed, this makes more sense. This is another instance of changing the
minimal amount to get it working. (In fact this is just the direct
result of running the automation script I wrote; it just puts it where
it "belongs" without regard to semantics.)
4 - on F17 with this patch set applied I get 'warning: Could not
retrieve fact ipaddress', the default configure invocation seems to run
successfully so am not sure if this affects anything or not (perhaps in
a profile which I did not use?)
That's almost certainly a facter bug. We don't use that fact presently,
so that's not a big deal for us.
After I pull your changes in I was able to build and install an update
aeolus-configure rpm on F17. The default invocation of configure didn't
seem to run successfully though (tried a few times to no avail) but I'm
not sure if this is due to an issue w/ configure or issues w/ the
various components being configured on F17. Regardless these patches
gets us closer to where we need to be so this is not a blocker.
I was seeing this too, it seems to be more a problem of the individual
components. Specifically, I was seeing some weird behavior with the
deltacloud-core sysv-style init script under systemd that was cascading
and causing everything else to fail. I changed it to run as a systemd
service unit and it worked fine after that, which is a good reminder
that I need to sync up with the deltacloud folks and get that remedied.
Invoking the updated configure on F16 works as it did before (I just
tried a few times w/ the default profile) so ACK provided this patchset
doesn't entail any functional changes, just reorganizing things.
(conductor-delayed_job did fail to start, but this was the same when I
tried starting it manually via the command line)
Cool. I'm going to hold off on pushing this until the aforementioned
"Various test and rake fixes for F17 compatibility" patches are
reviewed. Once those are ACKed I'll push both at the same time.
Thanks for looking at this Mo!