Actions
Task #3096
closedReview pete-fixes branch
Updated by Brett Smith almost 10 years ago
- Tracker changed from Bug to Task
- Assigned To set to Brett Smith
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 callingself.__contains__(item)
directly. Please also add a message to the KeyError that's raised.
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.
Updated by Brett Smith almost 10 years ago
4a78635a looks good to merge. Thanks.
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