Arvados

This branch looks very nice. My comments are all small stuff:

• The logic in PipelineInstancesController#create seems like it has some overlap with PipelineInstance#bootstrap_components, which is done before validation. Is it possible to assign the instance's template directly to the model, before we save, to reduce the amount of like code and avoid the second trip to the database?

• Does @incomplete_job in jobs_controller need to be an instance variable? It looks like it could stay in local scope, which could help ensure we don't try to reuse this implementation detail later.

• render_pipeline_component_attribute has two big blocks of if value_info.is_a? Hash back-to-back. Would it maybe be nicer to merge them? I really like a lot of the other readability improvements in here.

And one last point, less of a comment on your branch, but it brings up something that's been in the back of my mind: did you write the create_instance_long_enough_to test function to make the Workbench tests less side effect-ful on the API server? I feel pretty confident that we're going to consistently want to arranage better test isolation like this, so I wonder if this is something where it'd be worth putting in the up-front effort to write a general solution that people can use now and DTRT with.

• The logic in PipelineInstancesController#create seems like it has some overlap with PipelineInstance#bootstrap_components, which is done before validation. Is it possible to assign the instance's template directly to the model, before we save, to reduce the amount of like code and avoid the second trip to the database?

Yes! I've fixed the bootstrap_components method on the API server side so it actually works, and removed that stuff from the Workbench side.

• Does @incomplete_job in jobs_controller need to be an instance variable? It looks like it could stay in local scope, which could help ensure we don't try to reuse this implementation detail later.

Indeed. Fixed.

• render_pipeline_component_attribute has two big blocks of if value_info.is_a? Hash back-to-back. Would it maybe be nicer to merge them? I really like a lot of the other readability improvements in here.

Indeed. Merged.

And one last point, less of a comment on your branch, but it brings up something that's been in the back of my mind: did you write the create_instance_long_enough_to test function to make the Workbench tests less side effect-ful on the API server? I feel pretty confident that we're going to consistently want to arranage better test isolation like this, so I wonder if this is something where it'd be worth putting in the up-front effort to write a general solution that people can use now and DTRT with.

Yes. The database_cleaner gem seems to be aimed at this sort of thing. Perhaps apiserver just needs a button (which only functions when RAILS_ENV=test) that uses database_cleaner (or something) to reset to "just the fixtures" state.

Tom Clegg wrote:

• The logic in PipelineInstancesController#create seems like it has some overlap with PipelineInstance#bootstrap_components, which is done before validation. Is it possible to assign the instance's template directly to the model, before we save, to reduce the amount of like code and avoid the second trip to the database?

Yes! I've fixed the bootstrap_components method on the API server side so it actually works, and removed that stuff from the Workbench side.

Whoa… when I made that comment, I didn't realize just how much code we could shed. This is an amazing simplification. Thanks so much.

And one last point, less of a comment on your branch, but it brings up something that's been in the back of my mind: did you write the create_instance_long_enough_to test function to make the Workbench tests less side effect-ful on the API server? I feel pretty confident that we're going to consistently want to arranage better test isolation like this, so I wonder if this is something where it'd be worth putting in the up-front effort to write a general solution that people can use now and DTRT with.

Yes. The database_cleaner gem seems to be aimed at this sort of thing. Perhaps apiserver just needs a button (which only functions when RAILS_ENV=test) that uses database_cleaner (or something) to reset to "just the fixtures" state.

The Capybara documentation points to database_cleaner too, so I take that as a vote of confidence in it. A button like you describe would be a big help, but I don't think it's required to merge this branch; it can be its own thing. I think this branch is good to go.