-
Notifications
You must be signed in to change notification settings - Fork 173
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
docs: Polaris Admin Tool #830
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The doc change looks valuable for Polaris users so +1 in general.
I've got some minor comments, most of which are not about the doc, but about the tool itself 😅 Those can certainly be addressed in separate PR.
Usage: polaris-quarkus-admin-runner.jar bootstrap [-hV] [-c=<credentials>]... | ||
-r=<realms> [-r=<realms>]... | ||
Bootstraps realms and principal credentials. | ||
-c, --credential=<credentials> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: we should probably support reading secrets from the shell console and/or env. variables and/or files (as a follow-up PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, the tool is currently extremely rudimentary at this point. The idea of reading from stdin or file is excellent.
Bootstraps realms and principal credentials. | ||
-c, --credential=<credentials> | ||
Principal credentials to bootstrap. Must be of the | ||
form 'realm,clientId,clientSecret'. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
realm
has a dedicated option -r
🤔 does the value have to repeat it? It looks off from the usability perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks clumsy indeed, but the rationale is that you may want to bootstrap a realm without specifying credentials for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's usable :) and it's better to document a usable tool with rough edges, than not have a doc at all :)
Principal credentials to bootstrap. Must be of the | ||
form 'realm,clientId,clientSecret'. | ||
-h, --help Show this help message and exit. | ||
-r, --realm=<realms> The name of a realm to bootstrap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: realms
(plural) looks odd as a placeholder for a singular option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to look into picocli options to fix this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is fixed in the latest commit, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops yes, pushed one more commit.
|
||
## Purging Principal Credentials | ||
|
||
The `purge` command is used to remove realms and principal credentials from the Polaris server. This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we mention "realms" in the section title too? Could you add a paragraph on the implications of purging a realm?
```shell | ||
docker run apache/polaris-admin-tool:latest --help | ||
``` | ||
` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
` |
weight: 300 | ||
--- | ||
|
||
In order to help administrators manage their Polaris database, Polaris provides an administration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we be consistent with the name?
In order to help administrators manage their Polaris database, Polaris provides an administration | |
In order to help administrators manage their Polaris metastore, Polaris provides an administration |
As of January 2025, there is currently no binary release or official Docker image available for | ||
the tool. For now, you need to build the artifacts yourself, for example, by running the following | ||
command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to use a matched version for both polaris server and admin tool, even after the binary release. In that case, if a user/developer is using the main branch, they should compile the admin tool anyway.
As of January 2025, there is currently no binary release or official Docker image available for | |
the tool. For now, you need to build the artifacts yourself, for example, by running the following | |
command: | |
Make sure the admin tool and Polaris server are with the same version. If you are using Polaris from the source code, you need to build the artifacts yourself by running the following command: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll apply the suggestion, but for the record, while it is definitely better if server and tool have the exact same version, that is not a hard requirement. What needs to match is the metastore/database schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me. I think version matching is fine for a normal user, it's even harder for any user to check the metastore schema.
In order to help administrators manage their Polaris database, Polaris provides an administration | ||
tool. | ||
|
||
The tool is built using [Quarkus](https://quarkus.io/). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: do we need to call it out explicitly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? IMHO, it informs users about the technology stack (e.g. how to configure stuff).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just wanted to double-check if this is truly necessary. Typically, users don’t need to know the inner workings of a tool, especially since this document already explicitly states that configurations should remain consistent. Plus, there are a bunch of technologies behind it, we don't want to enumerate all of them. That said, it’s not a blocker, and I’m fine with proceeding either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove, I think it's pretty obvious already that Quarkus is being used.
To unpack and run the distribution, you can use the following command: | ||
|
||
```shell | ||
cd quarkus/admin/build/distributions | ||
unzip polaris-quarkus-admin-*.zip | ||
cd polaris-quarkus-admin-*/ | ||
java -jar polaris-quarkus-admin-*-runner.jar --help | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this unpack instruction? I'd prefer to leave for users/developers to figure it out by themselves if they want to do this.
Thanks @adutra for documenting this. I think all other places related to bootstrapping and purging are needed to be changed as well, includes but not limited to the boostramp section in the page of polaris-in-production https://github.com/polaris-catalog/polaris/blob/1369461eb98c79ade21a96110fbee6cca5c9a412/docs/configuring-polaris-for-production.md#L71-L71. I'm OK if we want to file another PR for these places, but I think it's still an urgent tasks to make sure all documents are consistent with the Quarkus change. cc @eric-maynard @collado-mike @dennishuo |
I agree that it would be awesome to have all documentation consistent with the Quarkus change. That is why I opened #700. But then I was asked to split that PR into smaller pieces. This PR is one of them. There is another PR being prepared for |
No description provided.