Skip to content

Replacing python quickstart with a CLI one #472

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

Merged
merged 13 commits into from
May 9, 2022
Merged

Conversation

cholmes
Copy link
Member

@cholmes cholmes commented May 2, 2022

This removes the python quickstart, which is available elsewhere in the docs, with a CLI one, as that's what we want to emphasize. This closes #453

@cholmes
Copy link
Member Author

cholmes commented May 2, 2022

@jreiberkyle & @sgillies - can you help me with the proper CLI command to wait for an order and download it when ready? The one example I have is:

$ planet orders wait 65df4eb0-e416-4243-a4d2-38afcf382c30 \
&& planet orders wait 65df4eb0-e416-4243-a4d2-38afcf382c30 

Which isn't right since it 'waits' on both calls. I tried out some variations with download but none seemed to do things right. If one of you could give the proper command here I can try it out and update the docs.

Copy link
Contributor

@jreiberkyle jreiberkyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for adding the auth stuff here. that was needed sorely. feel free to take my advice here as you wish. If you'd like, we can just update the create order content when the CLI change is implemented.

README.md Outdated

```
% planet orders create --name my-first-order --id <scene ids> \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The creation of the request and the creation of the order will be broken apart (this work is currently ticketed).

We can either walk the user through the process of creating a request or we can just provide them with a little request json example and go straight to creating the order. My preference is the latter since request creation is something that could evolve. Up to you.

Option 1:

My two suggestions for improvement here would be to add actual scene ids and to change the item type to PSScene since it is our current focus for item type and also some people may not have access to SkySat

$ planet orders request --name my-first-order \
  --id 20210821_161925_80_2453,20210821_161923_32_2453 \
  --item-type PSScene \
  --bundle visual

Option 2:

request.json (matches Option 1)

{  
   "name":"my-first-order",
   "products":[
      {  
         "item_ids":[  
            "20210821_161925_80_2453",
            "20210821_161923_32_2453"
         ],
         "item_type":"PSScene",
         "product_bundle":"visual"
      }
   ]
}

and then:

$ planet orders create request.json

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add actual scene ids

I feel like there's no scene ids that most or even many people would have? That was my hesitation, putting in a command that wouldn't work for most people because they don't have access. Putting a placeholder I hoped would make people actually try to change it to something that works. Perhaps we could use one that works in the dev trial at least? I wish open california still existed... Or that Planet admin system had an easy way to add data that anyone can access - could at least put the open data we have in STAC there.

PSScene since it is our current focus for item type

What do you mean by this? Just that we are pushing this as the 'new' thing?

I don't feel strongly, so am fine to switch to skysat.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and I definitely prefer option 1, give people the CLI instead of making them write json. But we don't have the 'request' thing implemented yet, right? I'd say we just merge this in and change it when that exists.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for merging now and fixing later: sounds good!

for scene ids: yeah, it would be awesome for us to have a 'starter pack' of scenes anyone with valid planet auth could access. In fact, is there a minimum set already established? My guess is 'no' but it would be great to find out.

PSScene: My thinking is that the primary sensor that people will use is a PS sensor, and all of the PS sensors are wrapped up in PSScene. Basically, let's align our help with what planet is messaging. Since PSScene was just released I figure it's probably being highlighted by planet.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there is a minimum set established. Maybe @sarasafavi knows? I think until there is one I'll just leave it in with a placeholder. Sounds good on PSScene.

cholmes and others added 4 commits May 3, 2022 18:25
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
% to $

Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
@cholmes cholmes marked this pull request as ready for review May 8, 2022 22:14
@cholmes
Copy link
Member Author

cholmes commented May 8, 2022

Ok, this is ready for final review. I can update the docs when #366 lands, but prefer to just get this one in.

Copy link
Contributor

@jreiberkyle jreiberkyle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lots of little nits but ultimately this looks great. You may want to remove the *.json files that got added along with the README

cholmes and others added 8 commits May 9, 2022 10:27
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
Co-authored-by: Jennifer Reiber Kyle <jreiberkyle@users.noreply.github.com>
@cholmes
Copy link
Member Author

cholmes commented May 9, 2022

Ok, I think I addressed everything, thanks for the great review!

@jreiberkyle
Copy link
Contributor

yep looks great!

@cholmes
Copy link
Member Author

cholmes commented May 9, 2022

Cool, merging in.

@cholmes cholmes merged commit d442cb5 into main May 9, 2022
@cholmes cholmes deleted the readme-quickstart-update branch May 9, 2022 18:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update readme for Orders CLI MVP milestone
2 participants