Arvados

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.

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?

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.

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.