Snow 1369491 snow object create command #1058
Conversation
| show_default=False, | ||
| ) | ||
| ObjectDefinitionArgument = typer.Argument( | ||
| help="""Object definition in JSON format, for example \'{"name": "my_database", "owner": "owner_role"}\', |
There was a problem hiding this comment.
Is this a valid example?
There was a problem hiding this comment.
I've checked - "owner" is not settable field, it is ignored by request (that's why my previous check passed). I've changed it to "comment"
| @@ -118,3 +121,55 @@ def test_show_drop_image_repository(runner, test_database, snowflake_session): | |||
| ) | |||
| assert result_drop.exit_code == 0, result_drop.output | |||
| assert f"{repo_name} successfully dropped" in result_drop.output | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize( | |||
There was a problem hiding this comment.
table, task, warehouse work when called manually, integration test role probably don't have enough permissions
There was a problem hiding this comment.
I think we should grant create for them and test it, but this will requires extending of our cleanup function. Can be done in separate task.
There was a problem hiding this comment.
| \'{"name": "my_db", "comment": "created with Snowflake CLI"}\', or provided as a list of key=value pairs, | ||
| for example: name=my_db 'comment=created with Snowflake CLI'. | ||
|
|
||
| Check https://docs.snowflake.com/LIMITEDACCESS/rest-api/reference/ for the full list of available parameters |
There was a problem hiding this comment.
Do not link to private docs, we should not merge the changes as long as APIs are not in PuPr.
There was a problem hiding this comment.
I left TODO to be filled, and changed base branch from main to object-create, which will collect PRs and merge them together into main when APIs will gave gone PuPr.
| ObjectAttributesArgument = typer.Argument( | ||
| None, | ||
| help="""Object attributes provided as a list of key=value pairs, | ||
| for example name=my_db 'comment=created with Snowflake CLI'. |
There was a problem hiding this comment.
why one has quotes and other does not?
There was a problem hiding this comment.
If the value contains spaces, it should be quoted. I just checked, comment='created with Snowflake CLI' works as well, changed.
| """Create an object of a given type. List of supported objects | ||
| and parameters: https://docs.snowflake.com/LIMITEDACCESS/rest-api/reference/""" | ||
| if object_attributes and object_json: | ||
| raise ClickException( |
There was a problem hiding this comment.
Loose though - I think we have a few cases like this, maybe we should have a ticket for introducing a common error?
There was a problem hiding this comment.
| object_data = { | ||
| v.key: v.value for v in parse_key_value_variables(object_attributes) | ||
| } |
There was a problem hiding this comment.
Any support for nested attirbutes?
There was a problem hiding this comment.
Nice catch, fixed
| no_retry=True, | ||
| ) | ||
|
|
||
| def _url_exists(self, url): |
| if object_type.endswith("y"): | ||
| plural_object_type = object_type[:-1].lower() + "ies" | ||
| else: | ||
| plural_object_type = object_type.lower() + "s" |
There was a problem hiding this comment.
Let's separate this to separate util method
| else: | ||
| plural_object_type = object_type.lower() + "s" | ||
|
|
||
| url_prefix = "/api/v2" |
There was a problem hiding this comment.
Make this a global constant , check for other usage in code base
There was a problem hiding this comment.
Fixed, this is the first use
|
|
||
| url = f"{url_prefix}/{plural_object_type}/" | ||
| if self._url_exists(url): | ||
| return url |
There was a problem hiding this comment.
Add comments explaining why this pre-check
There was a problem hiding this comment.
Added a docstring and refactored so it is more clear
|
|
||
| schema = self._conn.schema | ||
| url = f"{url_prefix}/databases/{db}/schemas/{schema}/{plural_object_type}/" | ||
| if self._url_exists(url): |
There was a problem hiding this comment.
Consider implementing an RestAPI class that would encapsulate the communication logic, we don't have to overload the object manger.
| if not response: | ||
| raise ClickException( | ||
| dedent( | ||
| """ An unexpected error occurred while creating the object. Try again with --debug for more info. |
There was a problem hiding this comment.
Won't this be duplicated in the output due to global handling?
There was a problem hiding this comment.
No, python connector surpresses the exception. I've added a "workaround", but I think it's worth to discuss with connector team to change that behaviour. IMO its worth to show info what exactly went wrong to the user
| ) | ||
| return response["status"] | ||
| except BadRequest: | ||
| raise ClickException("Incorrect object definition.") |
There was a problem hiding this comment.
Is there any information from the error that we can expose to the users? What exactly was wrong?
Imho we should show the original error but in more human-friendly way, for example not using json 😄
There was a problem hiding this comment.
No, the only information we get is "409 bad request", which, according to documentation, means malformatted payload (for example misspelled argument)
| ("image-repository", {"name": "test_image_repo"}), | ||
| ], | ||
| ) | ||
| @pytest.mark.integration |
There was a problem hiding this comment.
Add integration tests for expected errors so we have a check that nothing changes on server side
* update release-notes * Snow 1369491 snow object create command (#1058)
Pre-review checklist
Changes description
Add hidden "create" command, which is a thin layer over Snowflake REST API CREATE commands. Supported objects:
https://docs.snowflake.com/LIMITEDACCESS/rest-api/reference/index.html