Project

General

Profile

Actions

Task #3096

closed

Review pete-fixes branch

Added by Peter Amstutz almost 10 years ago. Updated almost 10 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Target version:
Actions #1

Updated by Brett Smith almost 10 years ago

  • Tracker changed from Bug to Task
  • Assigned To set to Brett Smith
Actions #2

Updated by Brett Smith almost 10 years ago

Reviewing 8c5ea8b5

  • In Workbench ActionsController, please refactor the two arv-normalize calls into a function, rather than duplicate the code.
  • Ditto for all the tempfile-rescuing in arv-edit.
  • My recollection from IRC is that the crunch-job umask isn't effective for Docker runs. Given that, what's your rationale for leaving it in?
  • I think this change in crunch-job will break non-Docker Job runs: $command .= "/tmp/crunch-src/crunch_scripts/" . $Job->{"script"}; (non-Docker runs are still based out of $ENV{CRUNCH_SRC})
  • I think new_request in Arvados.pm should check $self->{'noVerifyHostname'} (with the arrow).
  • StreamReader.manifest_text() has the regexp ^[0-9a-f]{32}\+(\d+)* What's going on with the optional group repetition at the end? Since the + in the group will be greedy, won't it capture all the digits and ensure that there's only one group?
  • Please remove debugging puts added to cancel_stale_jobs.rb.
  • In arvados_fuse, please refactor the debugging support so that every debug statement doesn't have to be a two-line stanza. The logging module has built-in support for different message levels and setting a minimum level for display at runtime.
  • In arvados_fuse MagicDirectory.__getitem__(), prefer item in self over calling self.__contains__(item) directly. Please also add a message to the KeyError that's raised.
Actions #3

Updated by Brett Smith almost 10 years ago

Reviewing 92aeef01

The Workbench tests give me this error:

  1) Error:
ApplicationControllerTest#test_get_10_objects_of_data_class_user:
ArgumentError: Argument is not a data class
    app/controllers/application_controller.rb:731:in `get_n_objects_of_class'
    test/functional/application_controller_test.rb:133:in `block in <class:ApplicationControllerTest>'

Please clean up the debug puts from the arv_normalize method, and the now-unnecessary __future__ import from the FUSE driver.

Actions #4

Updated by Peter Amstutz almost 10 years ago

Fixed.

Actions #5

Updated by Brett Smith almost 10 years ago

4a78635a looks good to merge. Thanks.

Actions #6

Updated by Anonymous almost 10 years ago

  • Status changed from New to Resolved
  • Start date set to 06/27/2014
  • % Done changed from 0 to 100
  • Remaining (hours) set to 0.0

Applied in changeset arvados|commit:377b4f345a5ba45bb96497a6be2b571604a695a1.

Actions

Also available in: Atom PDF