-
Notifications
You must be signed in to change notification settings - Fork 95
Support bulk subs creation in client and add --bulk to CLI to invoke it #1119
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
base: main
Are you sure you want to change the base?
Conversation
dd72894
to
ddc9ac7
Compare
I've now tested this via the CLI by running:
And I believe it works. Needs more testing though |
5c7111a
to
d962a6b
Compare
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.
Pull Request Overview
This PR adds support for bulk subscription creation to the client and CLI, allowing users to create multiple subscriptions at once using a feature collection reference. Key changes include:
- Implementation of the bulk_create_subscriptions method in both the synchronous and asynchronous clients.
- Addition of integration tests to verify bulk subscription creation.
- Update to the CLI to support the --bulk flag for invoking bulk creation.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
File | Description |
---|---|
tests/integration/test_subscriptions_api.py | Adds an integration test for bulk subscription creation. |
planet/sync/subscriptions.py | Introduces a synchronous bulk_create_subscriptions method. |
planet/clients/subscriptions.py | Adds an asynchronous bulk_create_subscriptions method using POST to /bulk. |
planet/cli/subscriptions.py | Adds support for the --bulk option in the subscriptions CLI command. |
d962a6b
to
607e62e
Compare
planet/cli/subscriptions.py
Outdated
@@ -151,6 +151,11 @@ async def list_subscriptions_cmd(ctx, | |||
|
|||
@subscriptions.command(name="create") # type: ignore | |||
@click.argument("request", type=types.JSON()) | |||
@click.option( | |||
"--bulk", |
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.
I'm split on whether or not the CLI bulk create path should be an optional flag on the standard create
argument as you have implemented here (e.g. create --bulk
), or if it should be it's own argument (e.g. bulk
,
bulk create
, bulk_create
, create_bulk
, etc).
Including an optional --bulk
flag on the standard create
command implies that the --bulk
flag modifies the behavior of the create
command. This isn't really the case though because bulk create has it's own API endpoint POST /subscriptions/v1/bulk
. Thinking forward, it might be difficult to enhance and/or modify the bulk create path in the CLI if tightly couple it with the standard create path.
Defining the bulk create path as a distinct argument implies that this is a distinct path, separate from the standard create path. Keeping the paths separate should make it easier to enhance and/or modify the bulk create CLI UX without getting wires crossed with the standard create path.
One more thing to think about is that the payloads for the two paths are slightly different. The standard create path expects a single object of subscription criteria, and the bulk create path expects an array of objects that define subscription criteria. I can see supporting two different request data structures for the same create
command, and driven by the --bulk
flag being a confusing UX.
Having typed this out and thought about it some more, I am leaning more towards the bulk create path having it's own distinct argument on account of the different endpoints and request data structures.
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.
That all makes sense to me! Update to break into a bulk-create subcommand. Let me know how it looks.
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! The distinct bulk-create
sub-command on the request
argument looks great! Could you also remove the --bulk
flag option from the create
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.
Oops! Thought I grabbed that but apparently not. Fixed.
9a09af2
to
3fcc7aa
Compare
d6b462d
to
f1d1602
Compare
LGTM! I left one more comment about removing the |
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.
Pull Request Overview
This PR adds support for bulk subscription creation in both the client and CLI while updating the test suites accordingly.
- Introduces a new client method bulk_create_subscriptions for both async and sync flows.
- Adds a new CLI command "bulk-create" for bulk subscription creation.
- Updates tests to validate the new bulk creation functionality.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
tests/integration/test_subscriptions_cli.py | Added tests for the new CLI bulk create command. |
tests/integration/test_subscriptions_api.py | Created async and sync tests for bulk create subscriptions. |
planet/sync/subscriptions.py | Added the sync bulk_create_subscriptions method. |
planet/clients/subscriptions.py | Added async bulk_create_subscriptions method with API error handling. |
planet/cli/subscriptions.py | Introduced a new CLI command for bulk create subscriptions. |
Comments suppressed due to low confidence (1)
planet/cli/subscriptions.py:234
- [nitpick] For consistency with the client method bulk_create_subscriptions, consider renaming the CLI command function to bulk_create_subscriptions_cmd.
async def bulk_create_subscription_cmd(ctx, request, pretty, **kwargs):
resp = pl.subscriptions.bulk_create_subscriptions({ | ||
'name': 'test', 'delivery': 'yes, please', 'source': 'test' | ||
}) | ||
assert '/subscriptions/v1?' in resp['_links']['list'] | ||
|
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.
The synchronous bulk_create_subscriptions method expects a list of dictionaries but is being passed a dictionary. Wrap the dictionary in a list to maintain consistency with the client API.
resp = pl.subscriptions.bulk_create_subscriptions({ | |
'name': 'test', 'delivery': 'yes, please', 'source': 'test' | |
}) | |
assert '/subscriptions/v1?' in resp['_links']['list'] | |
resp = pl.subscriptions.bulk_create_subscriptions([{ | |
'name': 'test', 'delivery': 'yes, please', 'source': 'test' | |
}]) |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
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.
Good catch, why didn't the test fail then? Hmm.
f1d1602
to
0ed8e82
Compare
@@ -247,6 +247,49 @@ planet subscriptions patch cb817760-1f07-4ee7-bba6-bcac5346343f \ | |||
patched-attributes.json | |||
``` | |||
|
|||
### Bulk Create Subscriptions |
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.
Nice! Thanks for adding the CLI documentation for bulk-create
!
Related Issue(s):
Closes #
Proposed Changes:
For inclusion in changelog (if applicable):
client.bulk_create_subscriptions()
which allows creating multiple subscriptions at once by referencing a feature collectionplanet subscriptions bulk-create
to bulk create using the above mechanismNot intended for changelog:
Diff of User Interface
Old behavior:
New behavior:
❯ planet subscriptions request-catalog --item-types PSScene --asset-types ortho_visual --geometry pl:features/my/adrian-test-new-guinea-10geojson-xqRXaaZ --start-time 2021-01-01 --end-time 2021-01-05 > catalog_fc_sub_source.json
❯ planet subscriptions request --name "bg test bulk sub" --source catalog_fc_sub_source.json > catalog_fc_sub.json
❯ planet subscriptions create --bulk --hosting sentinel_hub catalog_fc_sub.json
PR Checklist:
(Optional) @mentions for Notifications: