MonkeyCanCode commented on code in PR #4936:
URL: https://github.com/apache/polaris/pull/4936#discussion_r3502597694
##########
client/python/apache_polaris/cli/api_client_builder.py:
##########
@@ -199,7 +207,15 @@ def _build(self) -> ApiClient:
client_params["header_name"] = self.conf.header
client_params["header_value"] = self.conf.realm
- return ApiClient(config, **client_params)
+ api_client = ApiClient(config, **client_params)
+ # Attach direct catalog base (if provided via --catalog-url) so that
Review Comment:
The bare pass for exception is not very ideal, may be better to do following
instead:
```
api_client = ApiClient(config, **client_params)
if self.conf.explicit_catalog_url:
api_client.configuration._polaris_catalog_base =
self.conf.explicit_catalog_url
return api_client
```
##########
client/python/apache_polaris/cli/api_client_builder.py:
##########
@@ -84,7 +84,15 @@ def management_url(self) -> str:
@cached_property
def catalog_url(self) -> str:
Review Comment:
Maybe we should have a diff cached property for this instead?
```
@cached_property
def explicit_catalog_url(self) -> Optional[str]:
direct = self.options.catalog_url or
self.profile.get(Arguments.CATALOG_URL)
return direct.rstrip("/") if direct else None
@cached_property
def catalog_url(self) -> str:
return self.explicit_catalog_url or f"{self.base_url}/api/catalog"
```
##########
CHANGELOG.md:
##########
@@ -54,6 +54,7 @@ request adding CHANGELOG notes for breaking (!) changes and
possibly other secti
- Added `REGISTER_TABLE_OVERWRITE` authorization operation mapped to
`TABLE_FULL_METADATA` for deterministic overwrite authorization.
- Added Polaris Spark 4.0 client.
- Added `maintenance` support in Helm chart.
Review Comment:
We should also update
https://github.com/apache/polaris/blob/main/site/content/in-dev/unreleased/command-line-interface.md
##########
client/python/tests/test_parser_basic.py:
##########
@@ -171,6 +173,16 @@ def test_parse_argparse_valid_commands(self) -> None:
Parser.parse(
["--base-url", "https://customservice.com/subpath";, "catalogs",
"list"]
)
+ Parser.parse(
Review Comment:
Should we add unit test for actually simulate the new behavior?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
