Code review comment for lp:~canonical-platform-qa/autopilot/cpo-base-classes-fix

> Looking good guys, just a couple of comments inline.
>
> As per the changes I made to day; I was thinking about how to handle detecting
> and choosing the correct base class when there was multiple inheritance
> involved (i.e. multiple CPO inheritance).
>
> I came to the conclusion that it is bad form to change things under the cover
> in this fashion as it can be misleading and cause unexpected changes.

/me nods

> Instead I've opted for issuing a warning when this is detected with the option
> to upgrade this to raising an error in the new future (i.e. when we can be
> certain we don't break everyone with this new enforcement rule.)

I would prefer raising an error than fixing the base class too.

Misuse of the CPO is a bug, clients should be fixed.

Especially when looking at Leo's branches fixing this issue in several clients, the fix seems light enough that it's worth breaking the clients so they are fixed asap.

A warning may be missed and the client test suites will not be fixed until it's noticed.

>
> I was of 2 minds making it raise an exception straight off, but I feel that it
> was to heavy handed at this stage.
> What do you guys think?

Let's collect some test results to get a better understanding ;)

>
> I've done some small scale testing locally and the address book and unity8
> tests pass as expected. I would love to see this go through the gatekeeper to
> see the results there.

+1

Looking at http://q-jenkins.ubuntu-ci:8080/job/autopilot-release-gatekeeper it seems we can pass a PPA parameter (it doesn't have to be associated with a silo) so it should be possible to test the two approaches (warning/error) by building in 2 different PPAs and get an idea of the fallouts for both.