-
Notifications
You must be signed in to change notification settings - Fork 98
Implement CLI commands: planet data asset-get, download, activate, wait #864
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ne for the asset_download CLI command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EDIT:
Ok, wait, as I am looking at the usage in the PR desc I'm seeing an issue. If you save asset.json, then use planet data asset-download asset.json
, you're not going to be successful because asset.json
has not been updated since the asset was activated, and so it won't have the download urls. OK, let's go back to how the CLI-data docs specify the UI (that is, design-docs/CLI-Data.md), aka, input for the CLI commands are the item-type, item-id, and asset-type. This matches the orders UI as well.
INITIAL:
Ok so I tested this and found it to be a delight to use. The comments I have in the review are mostly fyis and style I think, I can't remember any necessary changes.
The one thing that DOES need to change is design-docs/CLI-Data.md
since the UI in this PR differs. Ideally, we'd be working from that doc and have discussed the different UI before you got to work here, but I'm not sure I called that out in the ticket for one, and for two, I am on board with this change. So, it should be a quick change but it's one we should get in there before moving forward. I'll approve when it's in!
Ok, wait, as I am looking at the usage in the PR desc I'm seeing an issue. If you save asset.json, then use |
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
@jreiberkyle alternatively, perhaps a better way of doing it would be: Or, maybe we stick to the CLI docs and then something like this works, too: I would just have to add the async with data_client(ctx) as cl:
asset = await cl.get_asset(item_type, item_id, asset_type_id)
path = await cl.download_asset(asset=asset,
filename=filename,
directory=Path(directory),
overwrite=overwrite,
progress_bar=not quiet)
if checksum:
cl.validate_checksum(asset, path) I think the later looks a little more elegant, let's go with it. |
@jreiberkyle Here’s what I did since you last reviewed it:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just the item-type
type needs to change to str (one entry) instead of comma-separated string (multiple entries)
For example, this is confusing to me:
planet data asset-activate PSScene,SkySatScene 20221003_002705_38_2461 basic_udm2
Error: {"general": [{"message": "Could not find the requested item."}], "field": {}}
For 1 how am I ordering the same ID/asset for two different satellites and for 2 the error message isn't telling me anything about how I specified two item types but only one will be used, and which one was used, which would help me understand the source of this error (maybe... I can't say I'm in love with the API error either).
… associated functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so close, so close! just three changes requested, two can be as simple as accepting suggestions, the other is an argument name change.
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm!
Related Issue(s):
Closes #505, #506, #507
Proposed Changes:
For inclusion in changelog (if applicable):
Not intended for changelog:
Diff of User Interface
Old behavior:
N/A
New behavior:
asset-activate
planet data asset-activate PSScene 20221003_002705_38_2461 basic_udm2
asset-wait
asset-download
Putting it all together
PR Checklist:
(Optional) @mentions for Notifications: