[PULL configure] Puppet 2.7 support v2

John Eckersberg jeckersb at redhat.com
Tue May 29 17:55:02 UTC 2012


On Mon, 28 May 2012 15:02:21 +0200, Mo Morsi <mmorsi at redhat.com> wrote:
> Hrm, one thing that I don't like about these pull requests is that it
> 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?)
> 

https://fedorahosted.org/pipermail/aeolus-devel/2012-May/010429.html

> 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 that
> 'db' is the folder which contains puppet modules for 'drop', 'seed',
> 'create', 'migrate'. I'd be fine w/ including this change in a follow-on
> patch though
> 

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

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!



More information about the aeolus-devel mailing list