Allow creating instances without waiting#80
Allow creating instances without waiting#80jvstme wants to merge 3 commits intoverda-cloud:masterfrom
Conversation
Add `InstancesService.create_nowait` method that returns immediately after sending a create request to the API.
|
Good idea! Can this be rewritten to support the same syntax as for clusters, i.e. ...instance.create(
# Set to None to not wait for provisioning but return immediately
wait_for_status=verda_client.constants.cluster_status.PROVISIONING
) |
|
@huksley, even with
Here are a few possible ways to address this:
Please let me know if any of these options seem reasonable, or if you have alternative suggestions. |
|
Would it be ok if instance.create() with with wait_for_status=None would return instance populated with ID and values passed as request payload? that way, no get instance method will be needed |
|
@huksley, |
|
@huksley, I’ve added the I also had to add support for callables in |
verda/instances/_instances.py
Outdated
| payload['pricing'] = pricing | ||
| id = self._http_client.post(INSTANCES_ENDPOINT, json=payload).text | ||
|
|
||
| if not wait_for_status: |
There was a problem hiding this comment.
Wouldn't this completely change logic because it will not wait for status but immediately return instance without wait_for_status set?
There was a problem hiding this comment.
By default, wait_for_status is set to lambda s: s != InstanceStatus.ORDERED, which means the method doesn't return here and nothing changes for existing users that are not setting wait_for_status.
For those users that will decide to set wait_for_status=None, the method will return here immediately, which is the expected behavior.
The implementation is consistent with ClustersService.create.
There was a problem hiding this comment.
Pull request overview
This PR updates instance creation to optionally skip polling/waiting for a status transition after the create request.
Changes:
- Added a
wait_for_statuskw-only parameter toInstancesService.createto control whether/how to wait for a desired status. - Updated polling logic to support either a specific status or a predicate callable.
- Added unit tests covering different
wait_for_statusbehaviors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| verda/instances/_instances.py | Adds wait_for_status parameter and adjusts create polling/early-return logic. |
| tests/unit_tests/instances/test_instances.py | Adds parameterized tests validating new waiting behavior for create. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pricing: Pricing | None = None, | ||
| coupon: str | None = None, | ||
| *, | ||
| wait_for_status: str | Callable[[str], bool] | None = lambda s: s != InstanceStatus.ORDERED, |
There was a problem hiding this comment.
The PR title/description says it adds InstancesService.create_nowait, but the diff only changes create() by introducing wait_for_status. Either add the described create_nowait method (e.g., a small wrapper calling create(..., wait_for_status=None)), or update the PR metadata to reflect the actual API change.
| pricing: Pricing | None = None, | ||
| coupon: str | None = None, | ||
| *, | ||
| wait_for_status: str | Callable[[str], bool] | None = lambda s: s != InstanceStatus.ORDERED, |
There was a problem hiding this comment.
wait_for_status is typed as str | Callable[[str], bool], but the code/tests appear to use InstanceStatus values (and instance.status is compared to InstanceStatus). This should be typed consistently (e.g., InstanceStatus | Callable[[InstanceStatus], bool] | None) to avoid misleading API contracts and type-checking issues.
There was a problem hiding this comment.
InstanceStatus is a namespace for str constants. The actual type passed to wait_for_status is str and not InstanceStatus
| if callable(wait_for_status): | ||
| if wait_for_status(instance.status): | ||
| return instance | ||
| elif instance.status == wait_for_status: | ||
| return instance |
There was a problem hiding this comment.
wait_for_status is typed as str | Callable[[str], bool], but the code/tests appear to use InstanceStatus values (and instance.status is compared to InstanceStatus). This should be typed consistently (e.g., InstanceStatus | Callable[[InstanceStatus], bool] | None) to avoid misleading API contracts and type-checking issues.
| if callable(wait_for_status): | |
| if wait_for_status(instance.status): | |
| return instance | |
| elif instance.status == wait_for_status: | |
| return instance | |
| status_value = getattr(instance.status, 'value', instance.status) | |
| if callable(wait_for_status): | |
| if wait_for_status(status_value): | |
| return instance | |
| else: | |
| target_status = getattr(wait_for_status, 'value', wait_for_status) | |
| if status_value == target_status: | |
| return instance |
There was a problem hiding this comment.
InstanceStatus is a namespace for str constants. The actual type passed to wait_for_status is str and not InstanceStatus
| contract: Optional contract type for the instance. | ||
| pricing: Optional pricing model for the instance. | ||
| coupon: Optional coupon code for discounts. | ||
| wait_for_status: Status to wait for the instance to reach, or callable that returns True when the desired status is reached. Default to any status other than ORDERED. If None, no wait is performed. |
There was a problem hiding this comment.
Grammar: change 'Default to' to 'Defaults to' for readability.
| wait_for_status: Status to wait for the instance to reach, or callable that returns True when the desired status is reached. Default to any status other than ORDERED. If None, no wait is performed. | |
| wait_for_status: Status to wait for the instance to reach, or callable that returns True when the desired status is reached. Defaults to any status other than ORDERED. If None, no wait is performed. |
There was a problem hiding this comment.
The current docstring is based on and consistent with ClustersService.create
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Add
wait_for_statusparameter toInstancesService.create, similar toClustersService.create. Settingwait_for_status=Noneallows to return from the method immediately.Resolves #79