On Fri, Jun 03, 2011 at 02:30:18PM +0200, Tomas Sedovic wrote:
The feature 1605 has 2 subtasks: 1610 (parse deployable definition)
and
1607 (replace the select deployable UI with a URL box for deployable).
Looks like this patch implements the latter but not the former. Neither
of the tasks are assigned in Redmine so I just want to make sure that's
what's going on.
Ah, sorry about the confusion. Without really intending to do so, I
ignored the subtasks and just worked on the parent task as a whole. 1607
is implemented here, as are a bunch of things that don't have tasks.
1610 is not done; my understanding is that this is still being ironed
out, and is one of the reasons that this interface doesn't actually
work.
There is 1 failing RSpec test and a 14 Cucumber features.
Yes, I was hoping for a sanity check here before going through and
cleaning up. Much of the old deployment code stops working; I didn't
want to remove all that code and the tests without a little reassurance
that I wasn't totally off base. I'll pull out some of the
now-inapplicable code and fix up the tests.
However, does this mean that we really no longer require a deployment
to
be attached to a deployable? Or is this just a temporary thing?
This is correct. The deployable_xml column in deployments will store a
chunk of XML that describes the deployable, as loaded from a URL,
instead of storing a separate Deployable object. (Actually parsing this
is something Scott's working on as we speak.)
Additionally, there are some concerns about pushing this before all
the
other pieces are in place as it would break launching deployments in the
meantime.
Yes, I had these concerns as well, and am not sure how to proceed. I was
encouraged to get this rolling, but I'm not sure how to proceed with
actually pushing it yet, as it does a lot of damage short-term. Does
anyone have any feedback here? I don't want to push this before it's
ready, but I also fear that if we wait until everything is ready, we'll
end up hopelessly deadlocked.
It looks to me that the _launch_new.haml and _new.haml partials are
virtually identical.
D'oh! They are, but _launch_new should just be deleted. I copied
_launch_new into _new and made that the place you go to, well, create a
new deployment.
The path '/conductor/deployments/check_name' doesn't work
when running
from a dev environment without the 'coductor' prefix.
Using "#{check_name_deployments_path}" would work no matter how it's set
up.
Oh yes, good catch. Thanks.
There is a race condition in this check.
Since the $.get requests are asynchronous and their time of delivery
over the network is not guaranteed, the uniqueness check result may not
reflect the name entered in the text field.
I'm inclined to go with your note about sticking a TODO disclaimer here.
It is a potential race condition, but I think the impact is very low --
worst case, someone steals your pool name before you can click "Next",
and when you do post the form, you get an error that the name is already
taken and must come up with another one.
-- Matt