Support configuration file for Polaris CLI #154

flyrain · 2024-08-16T06:48:07Z

Description

Support a configuration file for Polaris CLI

Type of change

New feature (non-breaking change which adds functionality)
This change requires a documentation update

How Has This Been Tested?

Manually test the following uses:

No config file
Correct config file
Invald config file
Incomplete config file, for example, no client id

Checklist:

I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation

sfc-gh-ygu · 2024-08-16T22:06:29Z

regtests/client/python/cli/polaris_cli.py

+
+        for attr in ['client_id', 'client_secret', 'host', 'port']:
+            if not getattr(options, attr) and config.get(attr):
+                setattr(options, attr, config[attr])


This doesn't overwrite the default values of host and port, will make the change to overwrite.

sfc-gh-ygu · 2024-08-16T22:07:31Z

regtests/client/python/cli/polaris_cli.py

+    @staticmethod
+    def _load_config():
+        try:
+            with open(POLARIS_CLI_JSON, "r") as file:


Only support the current location, need to support the home dir(~) as well.

probably want to allow a cli arg like -c configFile as well

eric-maynard · 2024-08-19T15:19:49Z

On a high level I'm not sure that I think this is necessary and I would probably rather us not support this. I have concerns like:

How/where are you documenting this functionality?
Why are the supported configs limited to only a small subset of flags?
How do users know how to map between the names used by command-line arguments and the ones in a config

flyrain · 2024-08-19T16:49:45Z

How/where are you documenting this functionality?

We can document it with an example, and explanation of each fields, like this page does for PyIceberg(https://py.iceberg.apache.org/configuration/#rest-catalog)

Why are the supported configs limited to only a small subset of flags?

It is the initial PR. We can enrich it over time.

How do users know how to map between the names used by command-line arguments and the ones in a config

Not all parameters will go to the config files
Let's keep the names consistent for the ones in both places, as well as document them.

I think it's worth to have it, if we consider Polaris Cli is a user-face tool, not only used for internal things like regression test. It is all about the ergonomic and better UX. For example, a user could have two Polaris catalogs to connect with, so that they can keep the config in a single file instead of inputing different parameter in the command line or switching the env variables every time. By saying that, this PR is not trying to have the full functionalities. It is a start point.

MonkeyCanCode · 2024-08-20T13:53:18Z

How/where are you documenting this functionality?

We can document it with an example, and explanation of each fields, like this page does for PyIceberg(https://py.iceberg.apache.org/configuration/#rest-catalog)

Why are the supported configs limited to only a small subset of flags?

It is the initial PR. We can enrich it over time.

How do users know how to map between the names used by command-line arguments and the ones in a config

Not all parameters will go to the config files

Let's keep the names consistent for the ones in both places, as well as document them.

I think it's worth to have it, if we consider Polaris Cli is a user-face tool, not only used for internal things like regression test. It is all about the ergonomic and better UX. For example, a user could have two Polaris catalogs to connect with, so that they can keep the config in a single file instead of inputing different parameter in the command line or switching the env variables every time. By saying that, this PR is not trying to have the full functionalities. It is a start point.

Requested changes pushed.

RussellSpitzer · 2024-08-26T01:37:06Z

regtests/client/python/cli/polaris_cli.py


+    @staticmethod
+    def _merge_with_config(options):
+        config = PolarisCli._load_config()


Could we do a quick rename of this to file_config or something like that? Just to make it clear the difference between options and config. The lines below are a little complicated to me otherwise

RussellSpitzer · 2024-08-26T01:39:16Z

regtests/client/python/cli/polaris_cli.py

+        if not config:
+            return
+
+        for attr in ['client_id', 'client_secret', 'host', 'port']:


Any reason why we want to allow-list only these props? Seems like we could allow anything here for future proofing?

I assume we could just have something like

for attr in config If attr not in options options[attr] = config[attr]

RussellSpitzer · 2024-09-23T20:50:39Z

@eric-maynard Could you follow up here? It looks like you had questions before on whether or not this should go forwards?

flyrain · 2024-09-23T20:54:14Z

Sorry, this is still in draft state. I'll close it, and reopen it once it's ready.

MonkeyCanCode · 2025-01-31T21:47:45Z

@flyrain thinking to take this one if we still want to support it (unless u are continue working on this?)

sfc-gh-ygu · 2025-01-31T21:49:47Z

I'm not working on it. Feel free to pick it up. I think it'd be nice to support it.

MonkeyCanCode · 2025-01-31T22:18:23Z

I'm not working on it. Feel free to pick it up. I think it'd be nice to support it.

Sounds good. Yeah, too many local scripts to support when using CLI without the concept of loading from a profile.

MonkeyCanCode · 2025-02-03T06:44:47Z

@sfc-gh-ygu here is a sample PR for this: #931

* Pass AccessConfig into FileIOFactory (apache#2937) it should not be the responsibility of the `FileIOFactory` to know how to infer the `AccessConfig` * Remove EclipseLink Persistence Backend (apache#2963) as per ML decision the deprecated eclipselink backend is being removed in the next version: https://lists.apache.org/thread/16bj5kngf2kfhqv3noxwfm7h9wlzvhyv * feat: Improve PolarisAdminTool default output (apache#2961) * feat: Improve PolarisAdminTool default output * feat: Improve PolarisAdminTool default output * Add Polaris Community Meeting 2025-10-30 (apache#2973) * Remove legacy management endpoints (apache#2276) * Build/nit: cache output of `generatedMarkdownDocs` (apache#2967) `(Java)Exec` tasks are not cacheable by default, as annotated with `@DisableCachingByDefault`. Adding an `outputs.cacheIf { true }` enables caching on those tasks. * Build: Helper to get effective ASF project metadata (apache#2969) This change "bundles" the information of `AsfProject` and the `PublishingHelperExtensions`, which is what the code in `configurePom.kt` did. Bundling these objects allows other consumers, like CycloneDX SBOM generation, to access that same information without having to query remote systems (whimsey.apache.org) again. * Alternative, concise PR template (apache#2945) This PR proposes an alternative PR template that is much shorter, and removes all the redundant claims. It also links to the contribution guidelines for further guidance. * Add docs how to add `Server` header to HTTP responses (apache#2941) * Prefer PolarisPrincipal over SecurityContext (apache#2932) The general idea is that `SecurityContext` comes from `jakarta.ws.rs` and there is no reason for non-REST related classes to rely on those details. Instead, once preprocessing of a REST-request has inferred the `PolarisPrincipal` all inner/core code should rely on only that. Note that this simplifies a bunch of tests that had to create their own `SecurityContext` around the principal that they wanted to use, thus having to decide how to implement `isUserInRole` and the other methods. * Last merged commit f934443 --------- Co-authored-by: Christopher Lambert <xn137@gmx.de> Co-authored-by: Yong Zheng <yongzheng0809@gmail.com> Co-authored-by: JB Onofré <jbonofre@apache.org> Co-authored-by: Alexandre Dutra <adutra@apache.org> Co-authored-by: fivetran-arunsuri <103934371+fivetran-arunsuri@users.noreply.github.com>

Support configuration file for Polaris CLI

402bd13

flyrain requested a review from a team as a code owner August 16, 2024 06:48

sfc-gh-ygu reviewed Aug 16, 2024

View reviewed changes

RussellSpitzer reviewed Aug 26, 2024

View reviewed changes

flyrain closed this Sep 23, 2024

Support configuration file for Polaris CLI #154

Support configuration file for Polaris CLI #154

Uh oh!

Conversation

flyrain commented Aug 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

sfc-gh-ygu Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

sfc-gh-ygu Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

eric-maynard commented Aug 19, 2024

Uh oh!

flyrain commented Aug 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MonkeyCanCode commented Aug 20, 2024

Uh oh!

RussellSpitzer Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RussellSpitzer commented Sep 23, 2024

Uh oh!

flyrain commented Sep 23, 2024

Uh oh!

MonkeyCanCode commented Jan 31, 2025

Uh oh!

sfc-gh-ygu commented Jan 31, 2025

Uh oh!

MonkeyCanCode commented Jan 31, 2025

Uh oh!

MonkeyCanCode commented Feb 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

flyrain commented Aug 16, 2024 •

edited

Loading

flyrain commented Aug 19, 2024 •

edited

Loading

RussellSpitzer Aug 26, 2024 •

edited

Loading