[PATCH conductor] Image building compatible with ActiveResource 3.2

Jiří Stránský jstransk at redhat.com
Tue Aug 21 10:17:12 UTC 2012


On 20.8.2012 18:05, Jason Guiditta wrote:
>
> I am not against checking for the version temporarily, we have that in
> a few places already.  However, I think it may make for less
> duplication (and more robust support) to put this check in our client
> library, aeolus-image.  Then we can leave the conductor codebase alone
> when we toss this in favor of the new rails engine martyn has been
> working on.  I also suspect that while this is only causing a few
> tests to fail, the same appraoch is needed for other calls to ARes
> that may not be well tested.  IOW, the tests may pass, but I bet we
> still end up with unexplained errors when doing other things with
> Factory of iwhd.  So, my suggestion is to make a small module in
> aeolus-image and mix it into the base classes for the factory and
> warehouse models.
>
> -j

Jan also proposed the change could only be made in aeolus-image. Originally, I didn't want to go with this because:

* We would have to make Image#initialize method behave like it was ActiveResource 3.0 when using ActiveResource 3.2. I didn't want to go "upgrade the lib to a newer version, but patch things so that they behave the old way".

* If we make the patch this way, it would have to stay patched forever. Later, we can remove forking based on ActiveResource version, but we cannot remove the patch, because if we still want to call Image.new(params_hash) in Conductor, we will still have to call super(params_hash, persisted_flag) in Image#initialize or ...::Factory::Base#initialize. If we want to remove the patch later, we'll have to go through all places where Image.new is called in Conductor anyway and change it like I changed it now.

All that said, I went through the rest of the calls to aeolus-image gem today and the issue seems to be present with more resources, like you said (all are Factory resources, because Warehouse resources apparently do not use ActiveResource). That makes the general solution more appealing.

So, patching the ...::Factory::Base#initialize should do. But I wouldn't make it a mixin, I'd change the method directly, because as I said above, I presume it would be a permanent solution. _What's your opinion on this?_

(I'll only set the persisted flag to true if params_hash[:id] is present, as there seem to be uses of ProviderImage.new even for cases where we really want to *create* a new resource and not just load it.)

Thanks for looking at the patch :)

J.



More information about the aeolus-devel mailing list