Fix polaris CLI support for updating catalog properties and hitting real Polaris services #404
Conversation
…eal Polaris services Add a --base-url argument that can be used instead of --host --port to support arbitrary Polaris URLs including https ones that have additional base prefixes before the /api/*/v1 subpaths. Fix namespaces.py to parse such URLs as well. Fix `catalogs update` to explicitly preserve default-base-location in the update request and use dict merge semantics for specified properties instead of just replacing the entire catalog contents with only what's specified in cli arguments, otherwise default-base-location or other properties get clobbered unexpectedly. Make the test_cli.py abide by POLARIS_HOST so that it can be pointed at a basic `./gradlew runApp` with `POLARIS_HOST=localhost` even without a docker container. Add tests for updating catalog properties.
dennishuo
requested review from
jbonofre,
ashvina,
RussellSpitzer,
snazy,
vvcephei,
takidau,
jackye1995,
flyrain,
eric-maynard,
collado-mike and
ebyhr
as code owners
Add checks for not specifying both --base-url and --host/--port Remove duplicate examples
|
I took another look at the namespaces command compared to the updated catalogs command, and I have one concern about this change. Currently, the other commands support an I am supportive of us changing all of the APIs to more of a "merge" style update (at the same time?), but in that case I would like to see us simultaneously add new APIs to actually remove properties / property-adjacent fields like default-base-location. |
…nds instead of using --property. This pattern matches Iceberg Spark's behavior for e.g. ALTER TABLE SET TBLPROPERTIES and ALTER TABLE UNSET TBLPROPERTIES where the SET command has "merge" semantics and thus UNSET is analogous to the separate --remove-property. This syntax is also consistent with other cloud providers' command-line tools such as `gcloud compute instances update --update-labels ... --remove-labels Implement consistent merge/remove pattern for catalogs/principals/principal-roles/catalog-roles Fix `namespaces create` to correctly merge --location with --property Fix `namespaces get` to print the full response json after calling the right load_namespace_metadata API instead of only calling namespace_exists Fix the handling of --base-url being exclusive to --host/--port
There was a problem hiding this comment.
Updated to have consistent merge behavior, introduced --set-property and --remove-property which is how other CLIs that follow the REST style have handled this case, such as gcloud compute instances update --update-labels ... --remove-labels ...: https://cloud.google.com/sdk/gcloud/reference/compute/instances/update
We probably don't need to be too worried about this being a "breaking" change to the CLI because:
catalogs updatenever even worked locally because of line 192 havingcatalog=cataloginstead ofproperties=....-- UpdateCatalogRequest doesn't actually have acatalogmember so the update doesn't workprincipals.pyon line 79 hadcurrent_entity_version=principal.current_entity_version,when there is no such member on the right-hand-side (it should beprincipal.entity_version) so it also errored out- No versioned release of Polaris yet anyways
| @@ -46,6 +46,8 @@ class CatalogRolesCommand(Command): | |||
| catalog_role_name: str | |||
| principal_role_name: str | |||
| properties: Optional[Dict[str, StrictStr]] | |||
There was a problem hiding this comment.
What does properties do now? Can we remove it?
There was a problem hiding this comment.
i.e. could you use set-property during create as well?
There was a problem hiding this comment.
My thinking for preserving --property for create is to emphasize the contrasting "declarative" semantics of CREATE vs the choice of having an "imperative" semantic for UPDATE. Looking at examples of other CLIs with a similar split, the same pattern is used:
gcloud compute instances create ... --labels=...
gcloud compute instances update ... --update-labels=... --remove-labels=...
And similarly --update-labels and --remove-labels aren't available in the create command, while --labels is not available in the update command.
There was a problem hiding this comment.
That makes sense. My initial hesitation was just having two properties that do the same thing.
Lets keep this, but I think we should reject the "invalid" inputs then:
catalogs update some_catalog --property k=v
catalogs update some_catalog --set-property k=v
There was a problem hiding this comment.
Agreed, this test case in test_cli_parsing.py covers the rejection of --property:
with self.assertRaises(SystemExit) as cm:
Parser.parse(['catalogs', 'update', 'catalog_name', '--property', 'foo=bar'])
self.assertEqual(cm.exception.code, INVALID_ARGS)
…hon-cli-for-catalog-updates-and-managed-polaris-services
…hon-cli-for-catalog-updates-and-managed-polaris-services
dennishuo
deleted the
dhuo-fix-python-cli-for-catalog-updates-and-managed-polaris-services
branch
Add a --base-url argument that can be used instead of --host --port to support arbitrary Polaris URLs including https ones that have additional base prefixes before the /api/*/v1 subpaths.
Fix namespaces.py to parse such URLs as well.
Fix
catalogs updateto explicitly preserve default-base-location in the update request and use dict merge semantics for specified properties instead of just replacing the entire catalog contents with only what's specified in cli arguments, otherwise default-base-location or other properties get clobbered unexpectedly.Make the test_cli.py abide by POLARIS_HOST so that it can be pointed at a basic
./gradlew runAppwithPOLARIS_HOST=localhosteven without a docker container.Add tests for updating catalog properties.
Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context. List any dependencies that are required for this change.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
./run.sh t_cli/test_cli.pyTest Configuration:
Checklist:
Please delete options that are not relevant.