Arvados

Reviewing d770a51

• I like the project+name additions, but I'm wondering about the approach and defaults. Would it make sense to inherit these options from arv-put the same way we get ones like --no-resume? Would it help if the default name started with "Docker image" or some other introductory term?
• The code adds a --push option, but never does anything with it. If we're going to determine the action by introspecting the image argument, I think that should just be the specified behavior and we should drop --push. And maybe we should just go all the way with this, and list when there's no image to put? I'm not totally sure what the right answer here is myself, but I don't think we should have an option that the code doesn't use, at least.
• Along the same lines, it should be an error to specify --list and an image argument.
• arg_parser.error() is a nicer way to report errors in the command line, like the educate method in the Ruby library.
• list_images_in_arv uses several single-letter variables. I don't mind this for short blocks and loops (like the one at the end), but for longer code it can be a real challenge to remember what letter is what. It also increases the odds of introducing a bug by accidentally reusing a letter. Please spell these out.
• I'm not sure "use local" at the end of the --no-pull help message will be clear enough to users who may not be so familiar with Docker. Please spell out a complete sentence here like "Use installed images only."
• Please keep the list of imports sorted.

Brett Smith wrote:

Reviewing d770a51

• I like the project+name additions, but I'm wondering about the approach and defaults. Would it make sense to inherit these options from arv-put the same way we get ones like --no-resume? Would it help if the default name started with "Docker image" or some other introductory term?

Not sure how inheriting the options works in this case. They can't be passed through transparently because in the case of --name it needs to override the default, and in the case of --project-uuid the keepdocker code needs to know the project as well.

• The code adds a --push option, but never does anything with it. If we're going to determine the action by introspecting the image argument, I think that should just be the specified behavior and we should drop --push. And maybe we should just go all the way with this, and list when there's no image to put? I'm not totally sure what the right answer here is myself, but I don't think we should have an option that the code doesn't use, at least.
• Along the same lines, it should be an error to specify --list and an image argument.
• arg_parser.error() is a nicer way to report errors in the command line, like the educate method in the Ruby library.

Dropped --push and --images in favor of listing images as the default behavior (or if the first positional parameter is 'images').

• list_images_in_arv uses several single-letter variables. I don't mind this for short blocks and loops (like the one at the end), but for longer code it can be a real challenge to remember what letter is what. It also increases the odds of introducing a bug by accidentally reusing a letter. Please spell these out.

Renamed to friendlier names.

• I'm not sure "use local" at the end of the --no-pull help message will be clear enough to users who may not be so familiar with Docker. Please spell out a complete sentence here like "Use installed images only."

Improved text.

• Please keep the list of imports sorted.

Fixed.

Peter Amstutz wrote:

Brett Smith wrote:

Reviewing d770a51

• I like the project+name additions, but I'm wondering about the approach and defaults. Would it make sense to inherit these options from arv-put the same way we get ones like --no-resume? Would it help if the default name started with "Docker image" or some other introductory term?

Not sure how inheriting the options works in this case. They can't be passed through transparently because in the case of --name it needs to override the default, and in the case of --project-uuid the keepdocker code needs to know the project as well.

That's fine, because option data that you inherit is still available through arg_parser and later args just like it is now. (Note that when we build put_args, we introspect opt_parser and not arg_parser—probably not the clearest variable naming on my part.) With this change, you'll just need to drop the option definitions and the else clause after if args.name is None. Everything else will continue to work as you've already written it.

All the rest of the changes look good. Thanks.

Reviewing b6017f4

• I wonder if it's really appropriate to unconditionally set ensure_unique_name=True in arv-put. I think it makes sense if we're using a default name; the user apparently doesn't care too much then. But if they provided a name and it's already taken, shouldn't we warn them and let them decide how to fix it themselves?
• The new code to "copy" Docker images sort of softly assumes that args.image will be a Docker repository. If the user provides a hash instead, the default name looks odd, the existing_repo_tag search shouldn't be run, and the code will usually create an incorrect docker_image_repo+tag link. I think the code should check for this and react appropriately, similar to the way the original code decides whether or not to make a docker_image_repo+tag link.
• Please clean up trailing whitespace and the unused textwrap import.

Thanks.

Reviewing 8eeb267

• In check_project_exists:
  • This function effectively forces the user to use except Exception for the not-found case. This seems less than ideal because except Exception will catch errors caused by genuine code bugs, like unexpected or mishandled types, divide-by-zero errors, etc.—which are more likely than usual when we're dealing with a dynamically generated API client. It would be nicer if there were a clear distinction between "the project was not found" and other kinds of errors. Raising a predictable exception class, probably ValueError, would be most Pythonic.
  • The name of the function should be updated to reflect its new semantics (returning the desired project UUID for the argument).
• The error "arv-put: Error adding Collection to project" used to be shown when there was a problem making a link; now it's shown when there's a problem making a collection. I think the text should be updated to better reflect this.

Thanks.

Brett Smith wrote:

Reviewing 8eeb267

• In check_project_exists:
  • This function effectively forces the user to use except Exception for the not-found case. This seems less than ideal because except Exception will catch errors caused by genuine code bugs, like unexpected or mishandled types, divide-by-zero errors, etc.—which are more likely than usual when we're dealing with a dynamically generated API client. It would be nicer if there were a clear distinction between "the project was not found" and other kinds of errors. Raising a predictable exception class, probably ValueError, would be most Pythonic.

Fixed.

• The name of the function should be updated to reflect its new semantics (returning the desired project UUID for the argument).

Fixed.

• The error "arv-put: Error adding Collection to project" used to be shown when there was a problem making a link; now it's shown when there's a problem making a collection. I think the text should be updated to better reflect this.

Fixed.

83bcea2 is good to merge. Thanks.