You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/08/20 22:44:38 UTC
[GitHub] [iceberg] rdblue opened a new pull request, #5594: Python: Fix CLI after properties refactor
rdblue opened a new pull request, #5594:
URL: https://github.com/apache/iceberg/pull/5594
The recent refactor of how properties work caused the CLI to stop working with REST catalogs that use tokens. The token was being fetched from properties using `self.property`, but was set in the constructor's local properties map. The two maps were different objects because properties are passed to `__init__` using `**properties`.
The result was a 400 error (missing `Authorization` header). This also fixes the `_fetch_config` method's error handling. It was not parsing the error payload and so the cause message was ignored. This also updates text output to pretty print the stack trace when an exception causes the CLI to fail.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#issuecomment-1221422097
@Fokko, FYI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950750936
##########
python/pyiceberg/cli/output.py:
##########
@@ -73,7 +73,7 @@ def _table(self) -> RichTable:
return RichTable.grid(padding=(0, 2))
def exception(self, ex: Exception):
- Console(stderr=True).print(ex)
+ Console(stderr=True).print_exception()
Review Comment:
This isn't ideal. Looks like there isn't a way to pass an exception in here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#issuecomment-1221612832
Thanks, @Fokko! Congratulations!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950889222
##########
python/pyiceberg/cli/output.py:
##########
@@ -134,6 +140,9 @@ def uuid(self, uuid: Optional[UUID]):
class JsonOutput(Output):
"""Writes json to stdout"""
+ def __init__(self, **properties: Any):
Review Comment:
I was wondering if we should do that since the parent is an abstract base class. Is that a good idea?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
Fokko commented on PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#issuecomment-1221612974
Thanks, and thank you for the PR! 👐🏻
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950889259
##########
python/tests/catalog/test_rest.py:
##########
@@ -64,14 +64,9 @@ def rest_mock(requests_mock: Mocker):
def test_no_uri_supplied():
- with pytest.raises(ValueError) as exc_info:
+ with pytest.raises(KeyError):
RestCatalog("production")
- assert (
Review Comment:
Yeah, that's what I thought, too. Best to test it there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950889172
##########
python/pyiceberg/cli/output.py:
##########
@@ -68,12 +68,18 @@ def uuid(self, uuid: Optional[UUID]):
class ConsoleOutput(Output):
"""Writes to the console"""
+ def __init__(self, **properties: Any):
+ self.verbose = properties.get("verbose", False)
Review Comment:
Fixed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950752230
##########
python/pyiceberg/cli/output.py:
##########
@@ -73,7 +73,7 @@ def _table(self) -> RichTable:
return RichTable.grid(padding=(0, 2))
def exception(self, ex: Exception):
- Console(stderr=True).print(ex)
+ Console(stderr=True).print_exception()
Review Comment:
I added a verbose option to make tests pass.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950889040
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -422,7 +424,6 @@ def list_namespaces(self, namespace: Union[str, Identifier] = ()) -> List[Identi
),
headers=self.headers,
)
- response.raise_for_status()
namespaces = ListNamespaceResponse(**response.json())
Review Comment:
Good catch!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950750875
##########
python/pyiceberg/catalog/__init__.py:
##########
@@ -133,23 +133,6 @@ def __init__(self, name: str, **properties: str):
self.name = name
self.properties = properties
- def property(self, key: str) -> str:
Review Comment:
I think it's best to remove this. There are places that bypass this check because the property is optional and `properties` are exposed to subclasses anyway. Since the subclasses need to pay attention to how `properties` are used, I think they should do that directly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko merged pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
Fokko merged PR #5594:
URL: https://github.com/apache/iceberg/pull/5594
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950750912
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -179,10 +179,9 @@ def __init__(
name: Name to identify the catalog
properties: Properties that are passed along to the configuration
"""
- super().__init__(name, **properties)
-
- self.uri = self.property("uri")
- if credential := self.properties.get("credential"):
+ self.properties = properties
Review Comment:
This bypasses the original problem because it doesn't set `self.properties` with a kwargs copy.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950751557
##########
python/pyiceberg/cli/output.py:
##########
@@ -73,7 +73,7 @@ def _table(self) -> RichTable:
return RichTable.grid(padding=(0, 2))
def exception(self, ex: Exception):
- Console(stderr=True).print(ex)
+ Console(stderr=True).print_exception()
Review Comment:
Maybe we should do this as a "verbose" option?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] Fokko commented on a diff in pull request #5594: Python: Fix CLI after properties refactor
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5594:
URL: https://github.com/apache/iceberg/pull/5594#discussion_r950804426
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -422,7 +424,6 @@ def list_namespaces(self, namespace: Union[str, Identifier] = ()) -> List[Identi
),
headers=self.headers,
)
- response.raise_for_status()
namespaces = ListNamespaceResponse(**response.json())
Review Comment:
We should move this one after the `raise_for_status` block, otherwise, we might consume the payload before checking if it is a 2xx. A non 2xx would have a different structure, and we would fail with a pedantic error.
##########
python/pyiceberg/cli/output.py:
##########
@@ -134,6 +140,9 @@ def uuid(self, uuid: Optional[UUID]):
class JsonOutput(Output):
"""Writes json to stdout"""
+ def __init__(self, **properties: Any):
Review Comment:
We could also move this to the constructor of the parent `Output` class, and then we don't have to implement it at both the `JsonOutput` and `ConsoleOutput`.
##########
python/pyiceberg/catalog/rest.py:
##########
@@ -179,10 +179,9 @@ def __init__(
name: Name to identify the catalog
properties: Properties that are passed along to the configuration
"""
- super().__init__(name, **properties)
-
- self.uri = self.property("uri")
- if credential := self.properties.get("credential"):
+ self.properties = properties
Review Comment:
Ah, thanks. Great catch! The problem here is that the `super().__init__` is only being called once, and it silently ignores the call on line 186 with the updated properties (that has the token).
##########
python/tests/catalog/test_rest.py:
##########
@@ -64,14 +64,9 @@ def rest_mock(requests_mock: Mocker):
def test_no_uri_supplied():
- with pytest.raises(ValueError) as exc_info:
+ with pytest.raises(KeyError):
RestCatalog("production")
- assert (
Review Comment:
This is okay with me, this is also covered by `tests.cli.test_console:test_missing_uri` which is user-facing.
##########
python/pyiceberg/cli/output.py:
##########
@@ -68,12 +68,18 @@ def uuid(self, uuid: Optional[UUID]):
class ConsoleOutput(Output):
"""Writes to the console"""
+ def __init__(self, **properties: Any):
+ self.verbose = properties.get("verbose", False)
Review Comment:
When we use class variables, it is best to also add it to the class itself:
```python
class ConsoleOutput(Output):
"""Writes to the console"""
verbose: bool
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org