dimas-b commented on code in PR #461:
URL: https://github.com/apache/polaris/pull/461#discussion_r1964844654


##########
polaris-core/src/main/java/org/apache/polaris/core/entity/PolarisEntityConstants.java:
##########
@@ -21,6 +21,10 @@
 public class PolarisEntityConstants {
 
   public static final String ENTITY_BASE_LOCATION = "location";
+
+  // the name of the root principal we create at bootstrap time
+  public static final String ROOT_PRINCIPAL_NAME = "root";

Review Comment:
   nit: is this change necessary?



##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/bootstrap/RootCredentialsSet.java:
##########
@@ -74,6 +76,54 @@ static RootCredentialsSet fromString(@Nullable String 
credentialsString) {
         : EMPTY;
   }
 
+  /**
+   * Parse a JSON object of credentials. Example: { "realm1": { "client-id": 
"client1",
+   * "client-secret": "secret1" }, "realm2": { "client-id": "client2", 
"client-secret": "secret2",
+   * "extra-field": "extra-value" } }

Review Comment:
   `fromUrl()` is already able to parse this kind of JSON... Why add a new 
parsing method? Why not reuse that code?



##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -43,20 +45,17 @@ static class InputOptions {
     FileInputOptions fileOptions;
 
     static class StandardInputOptions {
-
       @CommandLine.Option(
-          names = {"-r", "--realm"},
-          paramLabel = "<realm>",
-          required = true,
-          description = "The name of a realm to bootstrap.")
-      List<String> realms;
+          names = {"-c", "--credentials"},
+          description =
+              "Principal credentials to bootstrap. If provided, must be a 
valid JSON array e.g. "
+                  + "[{\"realm\": \"my-realm\", \"principal\": \"root\", 
\"clientId\": \"polaris\", \"clientSecret\": \"p4ssw0rd\"}]")
+      String credentials;
 
       @CommandLine.Option(
-          names = {"-c", "--credential"},
-          paramLabel = "<realm,clientId,clientSecret>",

Review Comment:
   What is the reason for removing the option for the user to specify plain 
text credentials as command line args?



-- 
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]

Reply via email to