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.
