Arvados

branch 18995-code-cleanup-1 at 437d0dc1dfeea7b81999caa1c540da0903df87f7 ready for review

developer-run-tests: #3049

The tests are failing which means that the change in addrIsLocal in lib/boot/supervisor.go (dropping the hardcoded return) is breaking. I guess that means we can drop the rest of the function instead, but it makes me wonder why we have that function, then.

branch 18995-code-cleanup-2 at b3919687f18582ccff1a6316846dcb04d9b5f989

developer-run-tests: #3043

Regarding this code:

    def run(self, runtimeContext):
        # ArvadosCommandTool subclasses from cwltool.CommandLineTool,
        # which calls makeJobRunner() to get a new ArvadosContainer
        # object.  The fields that define execution such as
        # command_line, environment, etc are set on the
        # ArvadosContainer object by CommandLineTool.job() before
        # run() is called.

        runtimeContext = self.job_runtime

In fact the passed-in runtimeContext and self.job_runtime are materially different. The first one is the "starting" runtime context at the top level of execution, the second one (job_runtime) is the runtime context at the point of execution where the job is created, and certain fields are modified or inherited by other parts of the workflow.

So the problem is that it is using the same name for two different things, which is largely due to way the code evolved. Suggest changing the name of the passed in parameter to make the code scanner happy:

    def run(self, toplevelRuntimeContext):

Peter Amstutz wrote:

Regarding this code:

[...]

In fact the passed-in runtimeContext and self.job_runtime are materially different. The first one is the "starting" runtime context at the top level of execution, the second one (job_runtime) is the runtime context at the point of execution where the job is created, and certain fields are modified or inherited by other parts of the workflow.

So the problem is that it is using the same name for two different things, which is largely due to way the code evolved. Suggest changing the name of the passed in parameter to make the code scanner happy:

[...]

Good idea, branch 18995-code-cleanup-2 at 7a556ab282e43101c7c5cbaee534f5f3062e8878

developer-run-tests: #3051

18995-addrislocal @ 954f3e357128f884409e52658933d120ce0127ea -- developer-run-tests: #3054

Tom Clegg wrote:

18995-addrislocal @ 954f3e357128f884409e52658933d120ce0127ea -- developer-run-tests: #3054

LGTM thanks!

• addrIsLocal not applicable after note-7
• the goups_controller thing should probably be "@object.destroy; show" instead of this

iff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index c1dc7a496..8539332f1 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -104,7 +104,6 @@ class Arvados::V1::GroupsController < ApplicationController
   def destroy
     if !TRASHABLE_CLASSES.include?(@object.group_class)
       return @object.destroy
-      show
     else
       super # Calls destroy from TrashableController module
     end

...since it's evidently trying to copy the behavior of ApplicationController before it was overridden by TrashableController, which is

  def destroy
    @object.destroy
    show
  end

(or... can we actually call ApplicationController.destroy intead of copying it?)

RailsAPI returns the deleted object in a successful response to a DELETE although I don't see any tests for that, let alone one for Groups that would have failed here.

Tom Clegg wrote:

18995-code-cleanup-1 LGTM except

• addrIsLocal not applicable after note-7
• the goups_controller thing should probably be "@object.destroy; show" instead of this

[...]

...since it's evidently trying to copy the behavior of ApplicationController before it was overridden by TrashableController, which is

[...]

(or... can we actually call ApplicationController.destroy intead of copying it?)

RailsAPI returns the deleted object in a successful response to a DELETE although I don't see any tests for that, let alone one for Groups that would have failed here.

Interesting, so destroying a filter or role group is broken, then. Indeed, I get an empty object back:

$ arv group create --group '{"group_class": "role"}'
{
 "created_at":"2022-04-15T14:14:36.487888873Z",
 "delete_at":null,
 "description":null,
 "etag":"6ub5fc9g94dn4lq0x9fnuattr",
 "frozen_by_uuid":null,
 "group_class":"role",
 "href":"/groups/ce8i5-j7d0g-acg9q8ddvcjgrz4",
 "is_trashed":false,
 "kind":"arvados#group",
 "modified_at":"2022-04-15T14:14:36.486183000Z",
 "modified_by_client_uuid":"ce8i5-ozdt8-g1uv539034n3e95",
 "modified_by_user_uuid":"ce8i5-tpzed-xo2k4i24bjzwedw",
 "name":"ce8i5-j7d0g-acg9q8ddvcjgrz4",
 "owner_uuid":"ce8i5-tpzed-000000000000000",
 "properties":{},
 "trash_at":null,
 "uuid":"ce8i5-j7d0g-acg9q8ddvcjgrz4",
 "writable_by":[
  "ce8i5-tpzed-000000000000000",
  "ce8i5-tpzed-xo2k4i24bjzwedw",
  "ce8i5-tpzed-xo2k4i24bjzwedw" 
 ]
}

$ arv group delete --uuid ce8i5-j7d0g-acg9q8ddvcjgrz4
{
 "created_at":null,
 "delete_at":null,
 "description":null,
 "etag":"",
 "frozen_by_uuid":null,
 "group_class":"",
 "href":"",
 "is_trashed":false,
 "kind":"arvados#group",
 "modified_at":null,
 "modified_by_client_uuid":null,
 "modified_by_user_uuid":"",
 "name":"",
 "owner_uuid":"",
 "properties":null,
 "trash_at":null,
 "uuid":"" 
}

I've corrected the bug as you suggested and added a test at 733e8c44dacf230d605fe6e5ea3f95f0abb8d823 on branch 18995-code-cleanup-1

developer-run-tests: #3061

This looks like it unintentionally makes an array of 1 hash:

...
    [group_class: "filter", properties: {"filters":[]}],
  ].each do |params|
...
      group = Group.create!(params).first

instead of:

    {group_class: "filter", properties: {"filters":[]}},

It works anyway, though, because create!() accepts an array to mean "create one object for each hash in array".

18995-code-cleanup-1 LGTM

Tom Clegg wrote:

This looks like it unintentionally makes an array of 1 hash:

[...]

instead of:

[...]

It works anyway, though, because create!() accepts an array to mean "create one object for each hash in array".

18995-code-cleanup-1 LGTM

Doh, thanks, merged with that fixed.

A number of further trivial fixes ready for review at 0ceaaf049f7042e54ccdafaded9fa4f7de4bdb8b on branch 18995-code-cleanup-3

developer-run-tests: #3063

18995-code-cleanup-3 LGTM

Tom Clegg wrote:

18995-code-cleanup-3 LGTM

Thanks! Merged.

9cc5ed8fb634fa4f22f7bbaed8de0e5052c89f87 on branch 18995-code-cleanup-4

developer-run-tests: #3065

Ward Vandewege wrote:

branch 18995-code-cleanup-2 at b3919687f18582ccff1a6316846dcb04d9b5f989

developer-run-tests: #3043

18995-code-cleanup-2 @ 7a556ab282e43101c7c5cbaee534f5f3062e8878

LGTM

Peter Amstutz wrote:

Ward Vandewege wrote:

branch 18995-code-cleanup-2 at b3919687f18582ccff1a6316846dcb04d9b5f989

developer-run-tests: #3043

18995-code-cleanup-2 @ 7a556ab282e43101c7c5cbaee534f5f3062e8878

LGTM

Thanks, merged!

A few missing alt tags added in 45498f07785ef3d22e6dd4b9bdee69220ed134e3 on branch 18995-code-cleanup-5

developer-run-tests: #3090

Merged into main.