Arvados

Branch 11185-wb-disable-reuse - e7b46691f
Test run: https://ci.curoverse.com/job/developer-run-tests/252/

Added "Re-run without reuse" button on the CR show page, passing params[:no_reuse] to the copy controller method.
When this happens, it passes '--disable-reuse' to the arvados-cwl-runner call.

Pending: Check what other cases should be covered.

Update: fed8ee006
Test run: https://ci.curoverse.com/job/developer-run-tests/253/

Updating cr.use_existing to false was missing when copying CR for a re-run.
Added related test.

• Can you rename “Re-run” as “Re-run with reuse”. You might also want to switch the two buttons. First “Re-run with reuse” and then “Re-run without reuse”

• Please update the tooltip on “Re-run with reuse” to say that the container will be reused

• Initializing command as follows might be better if there is a scenario such as params[:no_reuse] but src.command ! = ‘arvados-cwl-runner’ ?

+    command = src.command
     if params[:no_reuse]
       @object.use_existing = false
       # If "no reuse" requested, pass the correct argument to arvados-cwl-runner command.
@@ -71,8 +72,6 @@ class ContainerRequestsController < ApplicationController
         command = src.command - ['--enable-reuse']
         command.insert(1, '--disable-reuse')
       end
-    else
-      command = src.command
     end

Please merge the two Re-run buttons into one Re-run button with a popup (As we discussed in the Review meeting)

The popup presents the user with one option (a checkbox) to enable / disable reuse. Default is "checked" which means "reuse". Ok and Cancel buttons to close the popup. (You can alternatively use two radio buttons with the reuse being default). Please see pipeline instance "re-run options" for a similar usage.

Updates at: 65da23323
Test run: https://ci.curoverse.com/job/developer-run-tests/260/

Discovered that the previous default behavior of the Re-run button was to copy the CR with the reuse feature disabled, so this story is about allowing the user to enable reuse.
Replaced the additional Re-run button with a modal dialog that has a checkbox to enable the feature.
Also found a bug where the CR.use_existing attr was set to false by default but the command wasn't updated when used arvados-cwl-runner, so any subsequent CR created would be re-using jobs.

• link_to raw('<i class="fa fa-fw fa-play"></i> Re-run') => Please use the link title 'Re-run ...' to indicate that this presents a popup

• By default the copied CR won't be reusing jobs ... => since the reused objects are containers, can you say so instead of "jobs"?

• Not a big deal since we only have one checkbox currently, but "reset_form" is not working in the popup. Click on Re-use and click on Cancel and click on Re-use again and see that the checkbox is still checked.

• In test "container request copy with reuse enabled", can we also check that arvados-cwl-runner flag is set correctly? Also, can we check this flag in test "container request copy"?

Thanks.

Addressed all previous comments at d694a717a
Test run: https://ci.curoverse.com/job/developer-run-tests/263/

• It appears that you might be able to change the cmd for the completed-old CR fixture rather than adding another "completed-acr" fixture

• You might be able to simplify the name of the test “container request #{uses_acr} …” by removing the user_acr in the name

• In _extra_tab_line_buttons html, you might want to move the reset_form function to the bottom of the file outside the <% ... %> code to improve readability

Updates at c6452ff3c
Test run: https://ci.curoverse.com/job/developer-run-tests/265/

• Kept JS code inside the template's if statement so it's rendered only when necessary. As requested, changed the function's name to avoid conflicts.
• Removed the unnecessary completed-acr CR fixture and modified the command of completed-older to be useful for the added tests.