Added delete_engine_wait function that completes once an engine is completely deleted #129
Conversation
…leted, added tests for engine API
railib/api.py
Outdated
| @@ -356,8 +366,8 @@ def poll_with_specified_overhead( | |||
| time.sleep(duration) | |||
|
|
|||
|
|
|||
| def is_engine_term_state(state: str) -> bool: | |||
| return state == "PROVISIONED" or ("FAILED" in state) | |||
| def is_engine_term_state(state: str, targetState: str) -> bool: | |||
There was a problem hiding this comment.
I don't quite understand the change being made here, or at least I think this function name should change to be more clear what the intent is as it no longer just returns boolean 'if engine is in a term state'. Since "PROVISIONED" is passed in lower down, whats the point here?
There was a problem hiding this comment.
oh I caught it... DELETED is also used below.. so is DELETED not a term state? why wouldn't DELETED be added here?
There was a problem hiding this comment.
I think to be precise and to avoid a use case where you call create_engine but in response it says DELETED but you still think it got created. This is also how it's written in our Go SDK.
|
|
||
| if isinstance(rsp, list): | ||
| if len(rsp) == 0: | ||
| raise ResourceNotFoundError(f"Resource not found at {url}") |
There was a problem hiding this comment.
is it correct to say that historically an assertion error would be thrown instead of ResourceNotFound? (If so its good, just want to make sure we minor-version bump since surface area is changing)
There was a problem hiding this comment.
Nope, the assert was there to check if for some reason we get two resources with the same name. I don't think it was useful since the name is required to be unique for any resource. But even if we do get two resources with the same name for some reason the assert wouldn't do any good for the user as the user would just get confused.
The change is backward incompatible, previously the function would return an empty list if a requested resource not found. So any method like get_engine or get_database would return an empty list when we can't find a resource with the provided name. Ideally, our rest api should handle this and give 404 but since we don't have a rest api for it I emulate this behavior by throwing ResourceNotFoundError.
There was a problem hiding this comment.
got it, so we're replacing empty list with an error condition to match what a user of a rest api would expect (in this case that they get 1 resource and instead that resource doesn't exist)
…o properly use engine states to determine when the operation is complete
|
The work is postponed because during the implementation we realized that it requires more design from both the API service and SDK point of view. |
No description provided.