Arvados

Review feedback:

• doc/user/tutorials/running-external-program.html.textile.liquid
  • I think the phrase “create a new empty template” is a bit confusing, and can simply be “create an empty template”.
  • “Now add the following content:” - I am not quite sure if we should say clear the contents of the editor and add the following content?

• doc/user/tutorials/tutorial-new-pipeline.html.textile.liquid
  • “create a new empty template” => “create an empty template” (same as above)
  • Is there a link in the left nav or somewhere leading to this page or did we lose it recently?
  • I went to this page by typing the url directly and saw the following error.
    

    Replace the empty fields with the following content:
    Liquid error: No such template 'tutorial_bwa_pipeline'
    

• sdk/cli/bin/arv
  • subcommands = %w(keep pipeline tag ws edit create). Would it be too much to ask if we can list the commands alphabetically to aid readability?

• I also think that it would help greatly if the case statement switches are alphabetically ordered in the check_subcommands method. Only if it is not too much effort.

• def run_editor tmp, global_opts
  I think it would be great if tmp is renamed as tmp_file. I was curious whether it is tmp_dir or tmp_file on first glance and hence think a clearer name would be awesome. I do see that there are many places in this file that use the variable name “tmp” and hence totally cool if you prefer not to do it.

• When I used the project-uuid with a junk value, I was given the following message:
  Server responded 403: ["Owner uuid must be set to User or Group"]
  I think it would be better if we can make the error mesasge a bit more verbose and say something along the lines, no such uuid or permission denied etc. I do agree that is on the server side and not necessarily part of this update.

• I tested using a junk resource type name (saw error as expected), as well as with pipeline_template, group, and user and all worked as expected.

Radhika Chippada wrote:

Review feedback:

• doc/user/tutorials/running-external-program.html.textile.liquid
  • I think the phrase “create a new empty template” is a bit confusing, and can simply be “create an empty template”.
  • “Now add the following content:” - I am not quite sure if we should say clear the contents of the editor and add the following content?

Fixed.

• doc/user/tutorials/tutorial-new-pipeline.html.textile.liquid
  • “create a new empty template” => “create an empty template” (same as above)
  • Is there a link in the left nav or somewhere leading to this page or did we lose it recently?
  • I went to this page by typing the url directly and saw the following error.
    [...]

I updated this because I found it doing a search for uses of the old "create" command, but I think you're right that this page has been retired.

• sdk/cli/bin/arv
  • subcommands = %w(keep pipeline tag ws edit create). Would it be too much to ask if we can list the commands alphabetically to aid readability?

Fixed

• I also think that it would help greatly if the case statement switches are alphabetically ordered in the check_subcommands method. Only if it is not too much effort.

Fixed

• def run_editor tmp, global_opts
  I think it would be great if tmp is renamed as tmp_file. I was curious whether it is tmp_dir or tmp_file on first glance and hence think a clearer name would be awesome. I do see that there are many places in this file that use the variable name “tmp” and hence totally cool if you prefer not to do it.

Fixed

• When I used the project-uuid with a junk value, I was given the following message:
  Server responded 403: ["Owner uuid must be set to User or Group"]
  I think it would be better if we can make the error mesasge a bit more verbose and say something along the lines, no such uuid or permission denied etc. I do agree that is on the server side and not necessarily part of this update.

I prefer to leave it that was for now.

Peter, thanks for updating and I understand about the last one.
I think it would desirable that we remove the unused "doc/user/tutorials/tutorial-new-pipeline.html.textile.liquid".
LGTM with that. Thanks.