Arvados

Reviewing at 7c8e8d37:

• Since each component has its own output_name field, different components of the pipeline may have different names -- does it really make sense for PipelineInstance.friendly_link_name to just pick one at random for the whole instance? I'm concerned especially that because PipelineInstance.components is a hash and not an array (i.e. not ordered), then friendly_link_name won't necessarily return consistent results for the same pipeline instance over time.

• I'm finding it hard to follow what the difference is between current_job and pj are -- I would have thought that each item produced by render_pipeline_jobs would be a Job, but apparently pj[:job] is the Job and pj is something else. The fact that we're replacing current_job[:output] with pj[:output_uuid] is puzzling. This isn't exactly a request - just an observation that it's hard to tell whether the change is doing the right thing.

• If we no longer use the @stripped_manifest_lines or $stripped_manifest_text hack, can we get rid of that code entirely? That would be awesome.

Tim Pierce wrote:

Reviewing at 7c8e8d37:

apps/workbench/app/models/pipeline_instance.rb:

• Since each component has its own output_name field, different components of the pipeline may have different names -- does it really make sense for PipelineInstance.friendly_link_name to just pick one at random for the whole instance? I'm concerned especially that because PipelineInstance.components is a hash and not an array (i.e. not ordered), then friendly_link_name won't necessarily return consistent results for the same pipeline instance over time.

I added this because I wanted the Collection page to be able to link back to the pipeline that produced the collection output, with sensible link text. That code is checking to see if the pipeline instance has a empty name, and if so, it instead returns the name of the template that the pipeline was generated from. It doesn't have anything to do with the addition of output_name.

There was a bug where it wasn't checking if anything was returned from PipelineTemplate.where(), that's fixed now.

apps/workbench/app/views/pipeline_instances/_show_components_running.html.erb

• I'm finding it hard to follow what the difference is between current_job and pj are -- I would have thought that each item produced by render_pipeline_jobs would be a Job, but apparently pj[:job] is the Job and pj is something else. The fact that we're replacing current_job[:output] with pj[:output_uuid] is puzzling. This isn't exactly a request - just an observation that it's hard to tell whether the change is doing the right thing.

The job continues to store the manifest hash in output, however the pipeline instance component now stores output_uuid which is the collection created by arv-run-pipeline-instance. The change is to link to the actual collection that was created by the pipeline instance, rather than to the manifest hash which may be referenced by multiple jobs. Regarding pj, this code is a mess generally, hopefully it will be cleaned up in #3187

sdk/cli/bin/crunch-job

• If we no longer use the @stripped_manifest_lines or $stripped_manifest_text hack, can we get rid of that code entirely? That would be awesome.

You are absolutely correct, since we don't need to provide portable_data_hash along with the manifest text any more, then we don't need to do the calculation to hash the stripped md5 sum either. Deleted.

LGTM to merge. Thanks for this.