[PATCH conductor] Image building compatible with ActiveResource 3.2

Jason Guiditta jguiditt at redhat.com
Tue Aug 21 13:35:47 UTC 2012


On 21/08/12 12:17 +0200, Jiří Stránský wrote:
>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?_
>
As aeolus-image will no longer be used once the rails
engine/factory/v2 integration are done, I think keeping it isolated in
that gem makes the most sense.  You will have to make a smaller
number of changes to the code when compared to catching every instance
where conductor uses it, so it just feels like a more effective
approach to me.  If we were to keep this gem for some reason, I don't
see any reason the version check could not be removed in the future.
If we wanted to do that, we could just return a sensible error that
showed the proper api call, or better yet, just alias the method call
and allow either version to instantiate it (provided this doesn't
cause some security problem).  However, I don't expect we will have to
do this, as all the ARes stuff is currently built into the new engine.
Even if we split it out, I think it will probably end up as a new gem
rather than an update to the current one.

-j
>(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