dimas-b commented on code in PR #461:
URL: https://github.com/apache/polaris/pull/461#discussion_r1927943529
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisCredentialsBootstrap.java:
##########
@@ -63,6 +66,39 @@ public static PolarisCredentialsBootstrap
fromString(@Nullable String credential
: EMPTY;
}
+ /**
+ * Parse a JSON array of credentials. Example: """ [ {"realm": "a",
"principal": "root",
+ * "clientId": "abc123", "clientSecret": "xyz987"}, {"realm": "b",
"principal": "boot",
+ * "clientId": "boot-id", "clientSecret": "boot-secret"}, ] """
+ */
+ public static PolarisCredentialsBootstrap fromJson(String json) {
Review Comment:
Could you add a test for this method?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisCredentialsBootstrap.java:
##########
@@ -63,6 +66,39 @@ public static PolarisCredentialsBootstrap
fromString(@Nullable String credential
: EMPTY;
}
+ /**
+ * Parse a JSON array of credentials. Example: """ [ {"realm": "a",
"principal": "root",
+ * "clientId": "abc123", "clientSecret": "xyz987"}, {"realm": "b",
"principal": "boot",
+ * "clientId": "boot-id", "clientSecret": "boot-secret"}, ] """
+ */
+ public static PolarisCredentialsBootstrap fromJson(String json) {
+ ObjectMapper objectMapper = new ObjectMapper();
+ try {
+ List<Map<String, String>> credentialsList =
+ objectMapper.readValue(json, new TypeReference<List<Map<String,
String>>>() {});
+ List<String> formattedList = new ArrayList<>();
+ for (Map<String, String> entry : credentialsList) {
+ String realm = entry.get("realm");
+ String principal = entry.get("principal");
+ String clientId = entry.get("clientId");
+ String clientSecret = entry.get("clientSecret");
+ if (realm == null || principal == null || clientId == null ||
clientSecret == null) {
+ throw new IllegalArgumentException("Invalid JSON format for
credentials: " + json);
+ }
+ if (!principal.equals(PolarisEntityConstants.ROOT_PRINCIPAL_NAME)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Invalid principal %s. Expected %s.",
+ principal, PolarisEntityConstants.ROOT_PRINCIPAL_NAME));
+ }
+ formattedList.add(String.join(",", realm, principal, clientId,
clientSecret));
+ }
+ return fromList(formattedList);
Review Comment:
Re-parsing from a generated list looks too complex to me. Why not create a
`PolarisCredentialsBootstrap` object directly?
##########
polaris-core/src/main/java/org/apache/polaris/core/persistence/PolarisCredentialsBootstrap.java:
##########
@@ -63,6 +66,39 @@ public static PolarisCredentialsBootstrap
fromString(@Nullable String credential
: EMPTY;
}
+ /**
+ * Parse a JSON array of credentials. Example: """ [ {"realm": "a",
"principal": "root",
+ * "clientId": "abc123", "clientSecret": "xyz987"}, {"realm": "b",
"principal": "boot",
+ * "clientId": "boot-id", "clientSecret": "boot-secret"}, ] """
+ */
+ public static PolarisCredentialsBootstrap fromJson(String json) {
+ ObjectMapper objectMapper = new ObjectMapper();
+ try {
+ List<Map<String, String>> credentialsList =
+ objectMapper.readValue(json, new TypeReference<List<Map<String,
String>>>() {});
+ List<String> formattedList = new ArrayList<>();
+ for (Map<String, String> entry : credentialsList) {
+ String realm = entry.get("realm");
+ String principal = entry.get("principal");
+ String clientId = entry.get("clientId");
+ String clientSecret = entry.get("clientSecret");
+ if (realm == null || principal == null || clientId == null ||
clientSecret == null) {
+ throw new IllegalArgumentException("Invalid JSON format for
credentials: " + json);
+ }
+ if (!principal.equals(PolarisEntityConstants.ROOT_PRINCIPAL_NAME)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Invalid principal %s. Expected %s.",
+ principal, PolarisEntityConstants.ROOT_PRINCIPAL_NAME));
+ }
+ formattedList.add(String.join(",", realm, principal, clientId,
clientSecret));
+ }
+ return fromList(formattedList);
+ } catch (Exception e) {
+ throw new IllegalArgumentException("Failed to parse JSON: " + json, e);
Review Comment:
nit: an exception here may not be related to JSON parsing. I'd simply say
"unable to determine bootstrap credentials"... or something like that.
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -38,20 +39,37 @@ public class BootstrapCommand extends BaseCommand {
List<String> realms;
@CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
+ names = {"-c", "--credentials"},
+ description = "Principal credentials to bootstrap. Must be a valid JSON
array e.g. " +
+ "[{\"realm\": \"my-realm\", \"principal\": \"root\", \"clientId\":
\"polaris\", \"clientSecret\": \"p4ssw0rd\"}]")
+ String credentials;
+
+ @CommandLine.Option(
+ names = {"-p", "--print-credentials"},
description =
- "Root principal credentials to bootstrap. Must be of the form
'realm,clientId,clientSecret'.")
- List<String> credentials;
+ "Print root credentials to stdout")
Review Comment:
Also, could you add a test case for this to `BootstrapCommandTest`?
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -38,20 +39,37 @@ public class BootstrapCommand extends BaseCommand {
List<String> realms;
@CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
+ names = {"-c", "--credentials"},
+ description = "Principal credentials to bootstrap. Must be a valid JSON
array e.g. " +
+ "[{\"realm\": \"my-realm\", \"principal\": \"root\", \"clientId\":
\"polaris\", \"clientSecret\": \"p4ssw0rd\"}]")
+ String credentials;
+
+ @CommandLine.Option(
+ names = {"-p", "--print-credentials"},
description =
- "Root principal credentials to bootstrap. Must be of the form
'realm,clientId,clientSecret'.")
- List<String> credentials;
+ "Print root credentials to stdout")
+ boolean printCredentials;
@Override
public Integer call() {
warnOnInMemory();
+ if (credentials == null || credentials.isEmpty()) {
+ if (!printCredentials) {
+ throw new IllegalArgumentException("Specify either `--credentials` or
`--print-credentials` to ensure" +
Review Comment:
It's a simple error message. Stack trace is not valuable.
`spec.commandLine().getErr().println(...)` might be more user-friendly.
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -38,20 +39,37 @@ public class BootstrapCommand extends BaseCommand {
List<String> realms;
@CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
Review Comment:
while I agree that supporting well-formed JSON is valuable, I believe the
value is realized mostly in automated use case. This old option, however, is
easier to use for humans, IMHO... Why not keep both options available for users
to choose what it convenient for them?
For example: `--json` for JSON, and `--client-id`, `--client-secret` for
plain strings?
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -38,20 +39,37 @@ public class BootstrapCommand extends BaseCommand {
List<String> realms;
@CommandLine.Option(
- names = {"-c", "--credential"},
- paramLabel = "<realm,clientId,clientSecret>",
+ names = {"-c", "--credentials"},
+ description = "Principal credentials to bootstrap. Must be a valid JSON
array e.g. " +
+ "[{\"realm\": \"my-realm\", \"principal\": \"root\", \"clientId\":
\"polaris\", \"clientSecret\": \"p4ssw0rd\"}]")
+ String credentials;
+
+ @CommandLine.Option(
+ names = {"-p", "--print-credentials"},
description =
- "Root principal credentials to bootstrap. Must be of the form
'realm,clientId,clientSecret'.")
- List<String> credentials;
+ "Print root credentials to stdout")
Review Comment:
This option implies that credentials are generated, right? Could you mention
that in the description?
--
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]
