Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
admin-cli/src/main/java/cloud/katta/cli/commands/storage/minio/MinIOSTSStorage.java
Show resolved
Hide resolved
.../src/main/java/cloud/katta/cli/commands/hub/storageprofile/minio/MinIOSTSStorageProfile.java
Outdated
Show resolved
Hide resolved
chenkins
left a comment
There was a problem hiding this comment.
Add coverage for new command.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
admin-cli/src/main/java/cloud/katta/cli/commands/storage/minio/MinIOSTSStorage.java:65
--accessbucketPolicyName(and the backing fieldaccessbucketPolicyName) uses inconsistent casing compared to--createBucketPolicyName. For consistency and discoverability in CLI help, rename to--accessBucketPolicyName/accessBucketPolicyName. If the old flag was ever released, consider keeping--accessbucketPolicyNameas an additional alias to avoid breaking existing scripts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 7 comments.
Comments suppressed due to low confidence (1)
admin-cli/src/main/java/cloud/katta/cli/commands/storage/minio/MinIOSTSStorage.java:65
- The option was renamed from --createbucketPolicyName to --createBucketPolicyName, which is a breaking CLI change for existing scripts. Consider accepting both spellings (aliases) for backward compatibility, and also align the casing of --accessbucketPolicyName (e.g., add --accessBucketPolicyName) for consistency.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
admin-cli/src/main/java/cloud/katta/cli/commands/AbstractAuthorizationCode.java
Outdated
Show resolved
Hide resolved
...cli/src/main/java/cloud/katta/cli/commands/hub/storageprofile/s3/S3StaticStorageProfile.java
Show resolved
Hide resolved
.../src/main/java/cloud/katta/cli/commands/hub/storageprofile/minio/MinIOSTSStorageProfile.java
Show resolved
Hide resolved
...src/test/java/cloud/katta/cli/commands/hub/storageprofile/s3/S3StaticStorageProfileTest.java
Show resolved
Hide resolved
.../test/java/cloud/katta/cli/commands/hub/storageprofile/minio/MinIOSTSStorageProfileTest.java
Show resolved
Hide resolved
.../src/main/java/cloud/katta/cli/commands/hub/storageprofile/minio/MinIOSTSStorageProfile.java
Outdated
Show resolved
Hide resolved
…lient (cryptomator for client vs. cryptomatorhub for web)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try (HttpClient client = HttpClient.newHttpClient()) { | ||
| var response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
| if(response.statusCode() != 200) { | ||
| throw new IOException("Failed to fetch config from %s/api/config. HTTP status: %d".formatted(hubUrl, response.statusCode())); | ||
| } | ||
| var rootNode = new ObjectMapper().reader().readTree(response.body()); | ||
| this.tokenUrl = rootNode.get("keycloakTokenEndpoint").asText(); | ||
| this.authUrl = rootNode.get("keycloakAuthEndpoint").asText(); | ||
| } |
There was a problem hiding this comment.
HttpClient does not implement AutoCloseable, so try (HttpClient client = HttpClient.newHttpClient()) { ... } will not compile. Create the client without try-with-resources (or reuse a static client) and keep the try-with-resources only for resources that actually need closing.
| try (HttpClient client = HttpClient.newHttpClient()) { | |
| var response = client.send(request, HttpResponse.BodyHandlers.ofString()); | |
| if(response.statusCode() != 200) { | |
| throw new IOException("Failed to fetch config from %s/api/config. HTTP status: %d".formatted(hubUrl, response.statusCode())); | |
| } | |
| var rootNode = new ObjectMapper().reader().readTree(response.body()); | |
| this.tokenUrl = rootNode.get("keycloakTokenEndpoint").asText(); | |
| this.authUrl = rootNode.get("keycloakAuthEndpoint").asText(); | |
| } | |
| HttpClient client = HttpClient.newHttpClient(); | |
| var response = client.send(request, HttpResponse.BodyHandlers.ofString()); | |
| if(response.statusCode() != 200) { | |
| throw new IOException("Failed to fetch config from %s/api/config. HTTP status: %d".formatted(hubUrl, response.statusCode())); | |
| } | |
| var rootNode = new ObjectMapper().reader().readTree(response.body()); | |
| this.tokenUrl = rootNode.get("keycloakTokenEndpoint").asText(); | |
| this.authUrl = rootNode.get("keycloakAuthEndpoint").asText(); |
| var rootNode = new ObjectMapper().reader().readTree(response.body()); | ||
| this.tokenUrl = rootNode.get("keycloakTokenEndpoint").asText(); | ||
| this.authUrl = rootNode.get("keycloakAuthEndpoint").asText(); | ||
| } |
There was a problem hiding this comment.
rootNode.get("keycloakTokenEndpoint") / get("keycloakAuthEndpoint") can return null if the config response is missing those fields, leading to an NPE on .asText(). Use path(...) and validate non-empty values (or throw a clear exception) before proceeding.
| try { | ||
| Desktop.getDesktop().browse(uri); | ||
| } | ||
| catch(IOException e) { | ||
| // Ignore | ||
| } |
There was a problem hiding this comment.
Calling Desktop.getDesktop().browse(uri) can throw HeadlessException / UnsupportedOperationException (e.g., on servers/CI or minimal JREs). Guard with Desktop.isDesktopSupported() and catch broader runtime failures so the login flow doesn't crash in headless environments.
| try { | |
| Desktop.getDesktop().browse(uri); | |
| } | |
| catch(IOException e) { | |
| // Ignore | |
| } | |
| if(Desktop.isDesktopSupported()) { | |
| try { | |
| var desktop = Desktop.getDesktop(); | |
| if(desktop.isSupported(Desktop.Action.BROWSE)) { | |
| desktop.browse(uri); | |
| } | |
| } | |
| catch(IOException | RuntimeException e) { | |
| // Ignore | |
| } | |
| } |
| final ApiClient apiClient = new ApiClient(); | ||
| apiClient.setBasePath(hubUrl); |
There was a problem hiding this comment.
--hubUrl is no longer required (inherited from AbstractAuthorizationCode), but this command unconditionally uses hubUrl to build the API client base path. If the user supplies --accessToken without --hubUrl, this will fail at runtime. Consider making --hubUrl required for this command, or add an explicit validation with a clear error before calling the API.
| final ApiClient apiClient = new ApiClient(); | |
| apiClient.setBasePath(hubUrl); | |
| if (this.hubUrl == null || this.hubUrl.isBlank()) { | |
| throw new CommandLine.ParameterException( | |
| new CommandLine(this), | |
| "Missing required option: '--hubUrl'. This command requires a hub URL to build the API client." | |
| ); | |
| } | |
| final ApiClient apiClient = new ApiClient(); | |
| apiClient.setBasePath(this.hubUrl); |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public Void call() throws Exception { | ||
| final ApiClient apiClient = new ApiClient(); | ||
| apiClient.setBasePath(hubUrl); | ||
| apiClient.addDefaultHeader("Authorization", "Bearer %s".formatted(this.login())); | ||
| this.call(new StorageProfileResourceApi(apiClient)); | ||
| apiClient.setDebugging(debug); | ||
| final StorageProfileDto response = this.call(new StorageProfileResourceApi(apiClient)); | ||
| System.out.println(new JSON().getContext(null).writeValueAsString(response)); |
There was a problem hiding this comment.
--hubUrl is now declared in AbstractAuthorizationCode with required = false, but this command always calls apiClient.setBasePath(hubUrl). If the user provides --accessToken (so login() succeeds) but omits --hubUrl, this will fail at runtime. Please make --hubUrl required for storage profile commands (or add an explicit validation in call() before using it).
| public class ArchiveStorageProfile extends AbstractAuthorizationCode implements Callable<Void> { | ||
| @CommandLine.Option(names = {"--hubUrl"}, description = "Hub URL. Example: \"https://hub.testing.katta.cloud\"", required = true) | ||
| String hubUrl; | ||
|
|
||
| @CommandLine.Option(names = {"--uuid"}, description = "The uuid.", required = true) | ||
| @CommandLine.Option(names = {"--uuid"}, description = "Storage Profile.", required = true) | ||
| String uuid; |
There was a problem hiding this comment.
--hubUrl option was removed from this command, but the implementation still uses hubUrl (inherited from AbstractAuthorizationCode). Since --hubUrl is currently required = false in the superclass, it’s now possible to invoke storageprofile archive without providing a hub URL (especially when --accessToken is used), which will fail at runtime. Consider enforcing --hubUrl as required for this command (or validating it before apiClient.setBasePath(hubUrl)).
| public void testCall() throws ApiException { | ||
| final StorageProfileResourceApi api = Mockito.mock(StorageProfileResourceApi.class); | ||
| final UUID vaultId = UUID.randomUUID(); | ||
| final S3StaticStorageProfile cli = new S3StaticStorageProfile(vaultId.toString(), vaultId.toString(), "S3 static", "us-east-1", null, |
There was a problem hiding this comment.
In this unit test, the first constructor argument is hubUrl, but a random UUID string is passed instead. Even if it’s currently unused by call(api), it makes the test harder to understand and could break if the implementation starts relying on hubUrl. Prefer passing a realistic URL (or use the no-arg constructor and set fields explicitly).
| final S3StaticStorageProfile cli = new S3StaticStorageProfile(vaultId.toString(), vaultId.toString(), "S3 static", "us-east-1", null, | |
| final S3StaticStorageProfile cli = new S3StaticStorageProfile("https://hub.example.com", vaultId.toString(), "S3 static", "us-east-1", null, |
Uh oh!
There was an error while loading. Please reload this page.