Project

General

Profile

Actions

Bug #2596

closed

Workbench mangles script_parameters when starting a pipeline_instance

Added by Tom Clegg over 10 years ago. Updated over 10 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Story points:
1.0

Description

Pipeline template has a list as a default for one of its script_parameters.

Click "Create pipeline". Args all still look OK.

Click "Start". But now:


Subtasks 3 (0 open3 closed)

Task #2597: Fix handling of script_parameters when creating/editing a pipeline instanceResolvedTom Clegg04/12/2014Actions
Task #2598: Write tests for pipeline instance #create and #updateResolvedTom Clegg04/12/2014Actions
Task #2605: Review 2596-refactor-pipeline-create branchResolvedTom Clegg04/12/2014Actions
Actions #1

Updated by Tom Clegg over 10 years ago

  • Description updated (diff)
  • Assigned To set to Tom Clegg
Actions #2

Updated by Tom Clegg over 10 years ago

  • Status changed from New to In Progress
Actions #3

Updated by Brett Smith over 10 years ago

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.

Actions #4

Updated by Tom Clegg over 10 years ago

  • 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.

Actions #5

Updated by Brett Smith over 10 years ago

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.

Actions #6

Updated by Anonymous over 10 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 83 to 100

Applied in changeset arvados|commit:4d629923847cac25ed6bdaf5c96e1f33a1a8e0c8.

Actions

Also available in: Atom PDF