Feature #3453
closed[SDK] "arv keep docker" should support listing images in Keep and placing images in specific projects
Added by Peter Amstutz about 10 years ago. Updated almost 10 years ago.
Updated by Peter Amstutz about 10 years ago
- Subject changed from "arv keep docker" should support listing images in Keep and manipulating tags. to [SDK] "arv keep docker" should support listing images in Keep and manipulating tags.
- Category set to SDKs
Updated by Peter Amstutz almost 10 years ago
- Target version set to 2014-09-17 sprint
Updated by Peter Amstutz almost 10 years ago
- Assigned To set to Peter Amstutz
- Story points set to 1.0
Updated by Peter Amstutz almost 10 years ago
- Subject changed from [SDK] "arv keep docker" should support listing images in Keep and manipulating tags. to [SDK] "arv keep docker" should support listing images in Keep and placing images in specific projects
Updated by Brett Smith almost 10 years ago
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 theeducate
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.
Updated by Peter Amstutz almost 10 years ago
- Status changed from New to In Progress
Updated by Peter Amstutz almost 10 years ago
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 theeducate
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.
Updated by Brett Smith almost 10 years ago
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.
Updated by Brett Smith almost 10 years ago
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, theexisting_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.
Updated by Brett Smith almost 10 years ago
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 becauseexcept 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).
- This function effectively forces the user to use
- 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.
Updated by Peter Amstutz almost 10 years ago
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 becauseexcept 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.
Updated by Anonymous almost 10 years ago
- Status changed from In Progress to Resolved
- % Done changed from 0 to 100
Applied in changeset arvados|commit:f467b469109e27bc18635c8952892e4c23fabd60.