eric-maynard commented on code in PR #1799:
URL: https://github.com/apache/polaris/pull/1799#discussion_r2125452297
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -90,45 +90,41 @@ public Integer call() {
|| inputOptions.stdinOptions.credentials.isEmpty()
? RootCredentialsSet.EMPTY
:
RootCredentialsSet.fromList(inputOptions.stdinOptions.credentials);
- if (inputOptions.stdinOptions.credentials == null
- || inputOptions.stdinOptions.credentials.isEmpty()) {
- if (!inputOptions.stdinOptions.printCredentials) {
- spec.commandLine()
- .getErr()
- .println(
- "Specify either `--credentials` or `--print-credentials`
to ensure"
- + " the root user is accessible after bootstrapping.");
- return EXIT_CODE_BOOTSTRAP_ERROR;
- }
+ if (rootCredentialsSet.credentials().isEmpty()
+ && !inputOptions.stdinOptions.printCredentials) {
+ spec.commandLine()
+ .getErr()
+ .println(
+ "Specify either `--credentials` or `--print-credentials` to
ensure"
+ + " the root user is accessible after bootstrapping.");
+ return EXIT_CODE_BOOTSTRAP_ERROR;
}
}
- // Execute the bootstrap
Map<String, PrincipalSecretsResult> results =
metaStoreManagerFactory.bootstrapRealms(realms, rootCredentialsSet);
- // Log any errors:
boolean success = true;
for (Map.Entry<String, PrincipalSecretsResult> result :
results.entrySet()) {
- if (result.getValue().isSuccess()) {
- String realm = result.getKey();
+ String realm = result.getKey();
+ PrincipalSecretsResult secretsResult = result.getValue();
+ if (secretsResult.isSuccess()) {
spec.commandLine().getOut().printf("Realm '%s' successfully
bootstrapped.%n", realm);
- if (inputOptions.stdinOptions != null &&
inputOptions.stdinOptions.printCredentials) {
- String msg =
- String.format(
- "realm: %1s root principal credentials: %2s:%3s",
- result.getKey(),
-
result.getValue().getPrincipalSecrets().getPrincipalClientId(),
- result.getValue().getPrincipalSecrets().getMainSecret());
- spec.commandLine().getOut().println(msg);
+ if (inputOptions.stdinOptions != null
+ && inputOptions.stdinOptions.printCredentials
+ && !rootCredentialsSet.credentials().containsKey(realm)) {
Review Comment:
`--generate-credentials` seems redundant with the supplied credentials. That
is, there are 4 modes and 2 of them would be invalid:
| Credentials Provided | --generate-credentials | Valid |
| ------------- | ------------- | --- |
| false | false | true |
| false | true | false |
| true | false | false |
| true | true | true |
Besides that, we'd lose what appears to me a valid behavior -- echoing back
non-generated credentials.
In the contrived scenario where someone wants to bootstrap N different
realms, _and_ they want to generate credentials for a value in (0, N) of them,
_and_ they have an issue want to printing the non-generated credentials, they
have the option of simply running two bootstrap commands. I don't think we need
a breaking change to better support this scenario.
##########
quarkus/admin/src/main/java/org/apache/polaris/admintool/BootstrapCommand.java:
##########
@@ -90,45 +90,41 @@ public Integer call() {
|| inputOptions.stdinOptions.credentials.isEmpty()
? RootCredentialsSet.EMPTY
:
RootCredentialsSet.fromList(inputOptions.stdinOptions.credentials);
- if (inputOptions.stdinOptions.credentials == null
- || inputOptions.stdinOptions.credentials.isEmpty()) {
- if (!inputOptions.stdinOptions.printCredentials) {
- spec.commandLine()
- .getErr()
- .println(
- "Specify either `--credentials` or `--print-credentials`
to ensure"
- + " the root user is accessible after bootstrapping.");
- return EXIT_CODE_BOOTSTRAP_ERROR;
- }
+ if (rootCredentialsSet.credentials().isEmpty()
+ && !inputOptions.stdinOptions.printCredentials) {
+ spec.commandLine()
+ .getErr()
+ .println(
+ "Specify either `--credentials` or `--print-credentials` to
ensure"
+ + " the root user is accessible after bootstrapping.");
+ return EXIT_CODE_BOOTSTRAP_ERROR;
}
}
- // Execute the bootstrap
Map<String, PrincipalSecretsResult> results =
metaStoreManagerFactory.bootstrapRealms(realms, rootCredentialsSet);
- // Log any errors:
boolean success = true;
for (Map.Entry<String, PrincipalSecretsResult> result :
results.entrySet()) {
- if (result.getValue().isSuccess()) {
- String realm = result.getKey();
+ String realm = result.getKey();
+ PrincipalSecretsResult secretsResult = result.getValue();
+ if (secretsResult.isSuccess()) {
spec.commandLine().getOut().printf("Realm '%s' successfully
bootstrapped.%n", realm);
- if (inputOptions.stdinOptions != null &&
inputOptions.stdinOptions.printCredentials) {
- String msg =
- String.format(
- "realm: %1s root principal credentials: %2s:%3s",
- result.getKey(),
-
result.getValue().getPrincipalSecrets().getPrincipalClientId(),
- result.getValue().getPrincipalSecrets().getMainSecret());
- spec.commandLine().getOut().println(msg);
+ if (inputOptions.stdinOptions != null
+ && inputOptions.stdinOptions.printCredentials
+ && !rootCredentialsSet.credentials().containsKey(realm)) {
Review Comment:
`--generate-credentials` seems redundant with the supplied credentials. That
is, there are 4 modes and 2 of them would be invalid:
| Credentials Provided | --generate-credentials | Valid |
| ------------- | ------------- | --- |
| false | false | true |
| false | true | false |
| true | false | false |
| true | true | true |
Besides that, we'd lose what appears to me a valid behavior -- echoing back
non-generated credentials.
In the contrived scenario where someone wants to bootstrap N different
realms, _and_ they want to generate credentials for a value in (0, N) of them,
_and_ they have an issue with printing the non-generated credentials, they have
the option of simply running two bootstrap commands. I don't think we need a
breaking change to better support this scenario.
--
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: issues-unsubscr...@polaris.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
