On 07/05/2013 10:45 AM, kbon wrote:
> Please elaborate on how you see this as surprising in real usage, what
> problems do you envision?

It's just surprising, in general, to make an instance of a thing, and 
then make another instance of a thing, with the same parameters, the 
same way, and have it behave in a different way. That's why it wasn't 
clear to me that the change in order of the tests was significant. 
Mutable global state introduces an unexpected potential action at a 
distance.

I understand the use case now. I wasn't aware that in the presence of 
the necessary dependencies, puppet would wait to prefetch resources 
until after CRM has been installed. In this discussion you've done more 
to document the use case for `ready?` than I've been able to find. 
However, the comments in the code, the documentation, and the tests 
don't include any of that, but I think they should. It's unlikely future 
contributors will go through this PR discussion.

Still, I wonder if there's a better solution. Given that some 
dependencies on the corosync service are necessary anyway, is it 
possible to make that service not "done" until the service is really 
running? This way, Puppet's innate dependency handling will solve the 
problem, rather than this one-off kludge that is `ready?`.

Or, a more elegant structuring of the code would do a better job of 
taking this wart and sequestering it into one place. All of the 
providers have a lot of cut & paste code duplication that does the work 
of feeding some generated input to `crm`. It looks like you've already 
found most of these cases in #47, where you have several identical 
changes in multiple files like this:

     -        ENV['CIB_shadow'] = @resource[:cib
     +        setupShadow(@resource[:cib]) if @resource[:cib]

If that "take this thing I generated and feed it to `crm` function is 
moved into `corosync.rb`, common to all the providers, then you can, in 
just one place, change the logic to be:

- try feeding this to `crm`
- iff it fails:
    - try again, a few times

Then you can drop all the calls to `block_until_ready`, and you don't 
have the surprising mutable global state. You don't have the delay 
because you aren't even performing an additional ready check (the 
attempt to do the thing you wanted to do in the first place *is* the 
check), and you don't have the possibility that `crm` was ready in the 
past, but isn't now, but you have some global state incorrectly saying 
that it is. The tests are simplified, the provider implementations are 
simplified, and the hairiness is in just one place instead of peppered 
all over.