Code review comment for lp:~tribaal/charms/trusty/rabbitmq-server/fix-le-ignore-min-cluster

The added test coverage is appreciated, however we do need it to fit the methodology as described. To be clear, I'm not advocating for reducing test coverage.

To summarize the current behavior (and what to test):

"When min-cluster-size is set to a value which is higher than the actual number of rmq nodes:

Clustering is expected to fail when LE is *not* available (min not met);

Clustering is expected to succeed when LE is available (min ignored)."

^ This is indeed tricky, as all uosci tests are run with LE available, and specifying the juju version is outside the scope of amulet. I think (if that is indeed our desired behavior), a periodic mojo spec regression test is the path.

...

That said...

Given (config.yaml snippet):

  min-cluster-size:
    type: int
    default:
    description: |
      Minimum number of units expected to exist before charm will attempt to
      form a rabbitmq cluster

I'm not a fan of the existing behavior. If I am the user, and I tell the charm to have a minimum cluster size of 9, and only feed it 3 units, I would expect mayhem/failure (or to be blocked at the very least). IMO, we are being kind at the expense of the trust of a config option and the underlying hooks.

This behavior is what would make sense to me:

with LE, with status, when min < actual:
BLOCKED Require X units, only have N units

w/o LE, with status, when min < actual:
BLOCKED Require X units, only have N units

w/o LE, w/o status, when min < actual:
try, try, try, and eventually fail a hook