Project

General

Profile

Actions

Bug #18995

closed

code cleanup

Added by Ward Vandewege almost 2 years ago. Updated almost 2 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Story points:
-
Release relationship:
Auto

Description

Clean up the most pressing issues found by code analysis tools.


Files


Subtasks 3 (0 open3 closed)

Task #19009: review 18995-code-cleanup-1ResolvedWard Vandewege04/14/2022Actions
Task #19010: review 18995-code-cleanup-2ResolvedPeter Amstutz04/14/2022Actions
Task #19013: review 18995-code-cleanup-3ResolvedWard Vandewege04/15/2022Actions

Related issues

Related to Arvados - Support #19058: Add code scanning to jenkins pipelineNewActions
Actions #1

Updated by Ward Vandewege almost 2 years ago

  • Status changed from New to In Progress
Actions #2

Updated by Ward Vandewege almost 2 years ago

  • Description updated (diff)
Actions #3

Updated by Ward Vandewege almost 2 years ago

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.

Actions #5

Updated by Peter Amstutz almost 2 years ago

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):
Actions #6

Updated by Ward Vandewege almost 2 years ago

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

Actions #8

Updated by Ward Vandewege almost 2 years ago

Tom Clegg wrote:

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

LGTM thanks!

Actions #9

Updated by Tom Clegg almost 2 years ago

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
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.

Actions #10

Updated by Ward Vandewege almost 2 years ago

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

Actions #11

Updated by Tom Clegg almost 2 years ago

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

Actions #12

Updated by Ward Vandewege almost 2 years ago

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.

Actions #13

Updated by Ward Vandewege almost 2 years ago

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

developer-run-tests: #3063

Actions #14

Updated by Tom Clegg almost 2 years ago

18995-code-cleanup-3 LGTM

Actions #15

Updated by Ward Vandewege almost 2 years ago

Tom Clegg wrote:

18995-code-cleanup-3 LGTM

Thanks! Merged.

Actions #17

Updated by Peter Amstutz almost 2 years ago

Ward Vandewege wrote:

branch 18995-code-cleanup-2 at b3919687f18582ccff1a6316846dcb04d9b5f989

developer-run-tests: #3043

18995-code-cleanup-2 @ 7a556ab282e43101c7c5cbaee534f5f3062e8878

LGTM

Actions #18

Updated by Ward Vandewege almost 2 years ago

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!

Actions #19

Updated by Peter Amstutz almost 2 years ago

  • Target version changed from 2022-04-27 Sprint to 2022-05-11 sprint
Actions #20

Updated by Ward Vandewege almost 2 years ago

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

developer-run-tests: #3090

Merged into main.

Actions #21

Updated by Ward Vandewege almost 2 years ago

  • Status changed from In Progress to Resolved
Actions #22

Updated by Ward Vandewege almost 2 years ago

  • Related to Support #19058: Add code scanning to jenkins pipeline added
Actions #23

Updated by Ward Vandewege almost 2 years ago

Significantly improved metrics at https://sonarcloud.io/summary/overall?id=arvados_arvados, closing this ticket.

The plan (see #19058) is to do code scanning as part of the build pipeline.

Actions #24

Updated by Peter Amstutz almost 2 years ago

  • Release set to 51
Actions

Also available in: Atom PDF