Bug #4000
closed
[API] "re-run with latest" yields fiddlesticks if pipeline template has been updated with new components
Description
Attempting to re-run qr1hi-d1hrv-to0ukub4m8aqcg7 goes to
https://workbench.qr1hi.arvadosapi.com/pipeline_instances/qr1hi-d1hrv-to0ukub4m8aqcg7/copy?components=use_latest&pipeline_instance%5Bstate%5D=RunningOnServer&script=use_latest and produces:
Oh... fiddlesticks. Sorry, I had some trouble handling your request. undefined method `[]' for nil:NilClass
The cause here is that the pipeline template has been updated to include new components, so when PipelineInstancesController.copy attempts to copy the script_parameters for those components from the original instance, it throws an exception at https://arvados.org/projects/arvados/repository/revisions/9be9761666fd091496bee885b37598db9a6ab38a/entry/apps/workbench/app/controllers/pipeline_instances_controller.rb#L22:
srcvalue = source.components[cname][:script_parameters][pname]- the pipeline template includes components that are not present in the instance
- at least one of which includes a script_parameter that
- is a hash
- with a 'dataclass' field
- is a hash
- at least one of which includes a script_parameter that
Whew.
Short-term fix is to make sure that the copying process gracefully handles any unexpected changes between the template and the instance.
Updated by Tim Pierce over 10 years ago
- Subject changed from "re-run with latest" button yields fiddlesticks to [API] "re-run with latest" button yields fiddlesticks
- Description updated (diff)
- Category set to API
- Assigned To set to Tim Pierce
Updated by Tim Pierce over 10 years ago
- Subject changed from [API] "re-run with latest" button yields fiddlesticks to [API] "re-run with latest" yields fiddlesticks if pipeline template has been updated with new components
- Description updated (diff)
Updated by Brett Smith over 10 years ago
Reviewing 5233f1d, and I have only one minor suggestion: it might be nice if the new tests checked that the copied pipeline's foo parameter was sourced from the original instance's. That would help ensure that we copied as much as possible from the instance, even while pulling in new components from the template.
But, that's not directly required to fix the bug, so feel free to merge with or without that change. Thanks.
Updated by Tim Pierce over 10 years ago
Brett Smith wrote:
Reviewing 5233f1d, and I have only one minor suggestion: it might be nice if the new tests checked that the copied pipeline's foo parameter was sourced from the original instance's. That would help ensure that we copied as much as possible from the instance, even while pulling in new components from the template.
But, that's not directly required to fix the bug, so feel free to merge with or without that change. Thanks.
Yes, I think it's a good idea to test that. In fact, adding those assertions to the test forced me to update the test fixtures, which didn't adequately exercise the change (which applies only to script parameters that are hashes with 'dataclass' fields).
Updated the test and the fixtures. Code remains the same. The new test fails if the code is reverted. Can you take another peek and confirm that we think this accurately tests the thing being fixed?
Updated by Brett Smith over 10 years ago
Tim Pierce wrote:
Updated the test and the fixtures. Code remains the same. The new test fails if the code is reverted. Can you take another peek and confirm that we think this accurately tests the thing being fixed?
I do, thanks. My only comment now is that it seems helpful to do these assertions in both new functional tests (the patch only adds them to the second). But again, that's relatively minor, and I'm happy to see a merge either way. Thanks.
Updated by Tim Pierce over 10 years ago
Brett Smith wrote:
Tim Pierce wrote:
Updated the test and the fixtures. Code remains the same. The new test fails if the code is reverted. Can you take another peek and confirm that we think this accurately tests the thing being fixed?
I do, thanks. My only comment now is that it seems helpful to do these assertions in both new functional tests (the patch only adds them to the second). But again, that's relatively minor, and I'm happy to see a merge either way. Thanks.
Thanks for the nudge on the other test. Updated it with the same assertions, and merged. Thanks.
Updated by Tim Pierce over 10 years ago
- Status changed from New to Resolved
Applied in changeset arvados|commit:3ee8ac519f0c3f3fd211372d2a4699586d5c2aa8.