Story #3656

[SDKs] "arv create object_type" should create a new object and then open it like "arv edit" does now.

Added by Tom Clegg about 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
10/09/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Subtasks

Task #4170: Review 3656-arv-createResolvedPeter Amstutz

Task #4148: Implement 'arv create'ResolvedPeter Amstutz

Associated revisions

Revision debedb22
Added by Peter Amstutz almost 5 years ago

Merge branch '3656-arv-create' closes #3656

Revision d9eca6c6
Added by Peter Amstutz almost 5 years ago

Merge branch '3656-arv-create' closes #3656

History

#1 Updated by Peter Amstutz almost 5 years ago

  • Target version set to Arvados Future Sprints

#2 Updated by Ward Vandewege almost 5 years ago

  • Target version changed from Arvados Future Sprints to 2014-10-29 sprint

#3 Updated by Peter Amstutz almost 5 years ago

  • Assigned To set to Peter Amstutz

#4 Updated by Radhika Chippada almost 5 years ago

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.

#5 Updated by Peter Amstutz almost 5 years ago

  • Status changed from New to In Progress

#6 Updated by Peter Amstutz almost 5 years ago

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.

#7 Updated by Radhika Chippada almost 5 years ago

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.

#8 Updated by Anonymous almost 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 50 to 100

Applied in changeset arvados|commit:debedb222cfea4bfc4519032cad8475858e04034.

Also available in: Atom PDF