You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "vigneshio (via GitHub)" <gi...@apache.org> on 2026/06/30 09:26:40 UTC

[PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

vigneshio opened a new pull request, #4936:
URL: https://github.com/apache/polaris/pull/4936

   feat(cli): support custom IRC base URIs via --catalog-url (#4927)
   
   The CLI always appended `/api/catalog` (and `/v1`) on top of `--base-url`. This made it unusable when a proxy or deployment maps a custom path (e.g. `http://localhost:8181/server1`) directly to the Iceberg REST Catalog root.
   
   Solution:
   - Add `--catalog-url` (also works via profiles).
   - When set, it is used **verbatim** as the catalog host.
   - Falls back to `--base-url` + `/api/catalog` when not set (per reporter feedback).
   - `--access-token + --catalog-url` works for namespaces/tables/etc.
   - No server changes.
   ## Checklist
   - [x] Don't disclose security issues! (contact security@apache.org)
   - [x] Clearly explained why the changes are needed, or linked related issues
   - [x] Added/updated tests with good coverage, or manually tested (and explained how)
   - [x] Added comments for complex logic
   - [x] Updated `CHANGELOG.md` (if needed)
   - [ ] Updated documentation in `site/content/in-dev/unreleased` (if needed)


-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode merged PR #4936:
URL: https://github.com/apache/polaris/pull/4936


-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4936:
URL: https://github.com/apache/polaris/pull/4936#issuecomment-4880568324

   LGTM. Thanks @vigneshio 


-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
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: issues-unsubscribe@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on code in PR #4936:
URL: https://github.com/apache/polaris/pull/4936#discussion_r3501894177


##########
client/python/tests/test_polaris_cli.py:
##########
@@ -144,7 +142,9 @@ def test_enable_api_request_logging_logs_requests(
 
         mock_original_urlopen.assert_called_once()
         output = mock_stderr.getvalue()
-        self.assertIn("Request: GET http://localhost:8080/api/management/v1/catalogs", output)
+        self.assertIn(
+            "Request: GET http://localhost:8080/api/management/v1/catalogs", output

Review Comment:
   The formatting changes (incl the `assertIn` split) are from running `ruff format` for **client lint**. I kept them in a separate commit..  So if they're not needed for this PR means, we can easily drop that commit and handle the formatting separately. 
   



-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on code in PR #4936:
URL: https://github.com/apache/polaris/pull/4936#discussion_r3510082983


##########
client/python/tests/test_polaris_cli.py:
##########
@@ -144,7 +142,9 @@ def test_enable_api_request_logging_logs_requests(
 
         mock_original_urlopen.assert_called_once()
         output = mock_stderr.getvalue()
-        self.assertIn("Request: GET http://localhost:8080/api/management/v1/catalogs", output)
+        self.assertIn(
+            "Request: GET http://localhost:8080/api/management/v1/catalogs", output

Review Comment:
   + 1 for removing the formatting out from current 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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4936:
URL: https://github.com/apache/polaris/pull/4936#issuecomment-4849048502

   > Thanks for the quick fix, @vigneshio !
   > 
   > Changes LGTM with one concern: this PR has a lot of formatting changes. If they are required by our linting rules, let's put them in a separate PR to keep feature changes isolated, if you do not mind.
   > 
   > ... also wondering what @MonkeyCanCode thinks :)
   
   Thanks for the tagging. I will review this later tonight. 


-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4936:
URL: https://github.com/apache/polaris/pull/4936#discussion_r3500041425


##########
client/python/tests/test_polaris_cli.py:
##########
@@ -144,7 +142,9 @@ def test_enable_api_request_logging_logs_requests(
 
         mock_original_urlopen.assert_called_once()
         output = mock_stderr.getvalue()
-        self.assertIn("Request: GET http://localhost:8080/api/management/v1/catalogs", output)
+        self.assertIn(
+            "Request: GET http://localhost:8080/api/management/v1/catalogs", output

Review Comment:
   Are these reformatting changes necessary?



-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4936:
URL: https://github.com/apache/polaris/pull/4936#issuecomment-4849293792

   Thanks for the PR @vigneshio . Left couple comments.


-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "MonkeyCanCode (via GitHub)" <gi...@apache.org>.
MonkeyCanCode commented on PR #4936:
URL: https://github.com/apache/polaris/pull/4936#issuecomment-4861780887

   Thanks for the fix @vigneshio . I do agree with @dimas-b that we should avoid format change in this PR. Do you think we can revert those instead? Also, please fix the merge conflict then we will be good to go. 


-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #4936:
URL: https://github.com/apache/polaris/pull/4936#discussion_r3503350740


##########
client/python/tests/test_polaris_cli.py:
##########
@@ -144,7 +142,9 @@ def test_enable_api_request_logging_logs_requests(
 
         mock_original_urlopen.assert_called_once()
         output = mock_stderr.getvalue()
-        self.assertIn("Request: GET http://localhost:8080/api/management/v1/catalogs", output)
+        self.assertIn(
+            "Request: GET http://localhost:8080/api/management/v1/catalogs", output

Review Comment:
   From my personal POV, it's preferable to isolate pure formatting changes from feature changes.
   
   However, I'm probably going to be away for a few days :slightly_smiling_face: Please consider my comments non-blocking and follow @MonkeyCanCode 's direction.



-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] feat(cli): support custom IRC base URIs via --catalog-url [polaris]

Posted by "vigneshio (via GitHub)" <gi...@apache.org>.
vigneshio commented on PR #4936:
URL: https://github.com/apache/polaris/pull/4936#issuecomment-4854460913

   Thanks @MonkeyCanCode for the detailed review . 😊 
   
   Added the simulation tests + `explicit_catalog_url `+ if refactor in the latest commit. Also updated the docs... PTAL.


-- 
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@polaris.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org