-
Notifications
You must be signed in to change notification settings - Fork 95
Sync client base_url option #1171
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-3.0-dev
Are you sure you want to change the base?
Conversation
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 base URL configuration support to the synchronous Planet client, bringing feature parity with the async clients and CLI. The changes enable users to override default API endpoints for different Planet services when using the sync client.
Key changes:
- Updated Planet sync client constructor to accept optional base URL parameters for each API service
- Added "invalid" subscription status to CLI options for completeness
- Included unit tests to verify the base URL functionality works correctly
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
planet/sync/client.py | Added optional base_url parameters to Planet constructor and passed them to underlying API clients |
planet/cli/subscriptions.py | Added missing "invalid" status to subscription status filter options |
test_base_url_functionality.py | Added comprehensive unit tests to verify custom and default base URL behavior |
planet/sync/client.py
Outdated
data_base_url: Optional[str] = None, | ||
orders_base_url: Optional[str] = None, | ||
subscriptions_base_url: Optional[str] = None, | ||
features_base_url: Optional[str] = None) -> None: |
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.
We should be mindful that this will become part of the SDK API that users are presented with, but users may not have a need for this level of configuration. We should try to keep our API surface clean and simple if possible.
Another approach could be to accept a single, common base_url
(defaulting to https://api.planet.com
) and each class under the hood (DataAPI, OrdersAPI) etc can add the appropriate path suffix e.g. /data/v1
.
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.
Agreed a single configuration would be ideal. It would also be nice to allow specification via CLI.
Also, since the CLI uses the async clients, we'd need to alter the approach taken there of statically building the URLS (like in data.py) based off of the constants
module
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 ideas! Updated to simplify the signature to only allow a single base_url
.
6afe262
to
0b145d9
Compare
This MR updates the
Planet()
sync client with a kwarg option to override the default base url for different client APIs. Support for configuring a custom base_url already exists for the async clients and CLI, this MR just brings that feature parity to the sync client. Even though this is a trivial change, I am including some unit tests to demonstrate that this functionality works and exists rather than updating the public docs because I think this feature holds more value for internal Planet workflows rather than customer workflows.I also noticed that the list of subscription statuses in the CLI was missing the new
invalid
status, so I added that in this MR.Notes from Claude:
I have successfully completed GitHub issue #1143 "Fix base URL handling". Here's what was implemented:
✅ CLI Support
The CLI already had --base-url options implemented for all API commands:
✅ SDK Support
Updated the synchronous Planet client in planet/sync/client.py to support configurable base URLs:
from planet import Planet
Use custom base URLs for specific APIs
pl = Planet(
data_base_url="https://staging-data.planet.com/data/v1",
orders_base_url="https://staging-orders.planet.com/compute/ops"
)
Or use defaults (production)
pl = Planet()
✅ Backward Compatibility
All changes maintain full backward compatibility - existing code continues to work without modification.
✅ Implementation Details
The implementation satisfies all acceptance criteria: