Java V2 Add Inspector examples #7660
Conversation
scmacdon
added
Java-v1
github-actions
bot
added
Java-v2
scmacdon
changed the title
debora-ito
left a comment
debora-ito
left a comment
There was a problem hiding this comment.
Just left one comment, looks good otherwise.
|
|
||
| | Action | Exception | Handling | | ||
| |-------------------------------|---------------------------|--------------------------------------------------------------------------| | ||
| | `Enable` | `ValidationException` | Prints a message indicating Inspector may already be enabled. | |
There was a problem hiding this comment.
The table should only include one specific exception for each action, not the more generic Inspector2Exception (which also looks like a Java class).
| | `ListCoverageStatistics` | `Exception` | Wraps and throws a `RuntimeException` for unexpected errors. | | ||
| | `ListUsageTotals` | `ValidationException` | Prints validation error details. | | ||
| | `ListUsageTotals` | `Inspector2Exception` | Prints AWS service error details and rethrows the exception. | | ||
| | `ListUsageTotals` | `Exception` | Wraps and throws a `RuntimeException` for unexpected errors. | |
There was a problem hiding this comment.
Is this action even used? I don't see it in the metadata.
There was a problem hiding this comment.
Both operations are in metadata - ListCoverageStatistics and ListUsageTotals
| System.out.println("Step 2: Ensuring Inspector is enabled..."); | ||
|
|
||
| try { | ||
| inspectorActions.enableInspector(inspectorClient, null); |
There was a problem hiding this comment.
There is a try and a catch in each of the inspectorAction methods, so why do we need an additional try/catch here? Shouldn't there be one try/catch around the scenario with a cleanup in the catch? This applies to all the steps in the scenario.
| inspectorActions.getAccountStatus(inspectorClient); | ||
|
|
||
| } catch (Exception e) { | ||
| System.err.println(" Could not create example filter: " + e.getMessage()); |
There was a problem hiding this comment.
These error messages don't match the actions called? Seems to be the same error message (copy/paste?) for all the actions. But I'm not sure they are needed anyway - see my other comment about the try/catches.
There was a problem hiding this comment.
Code has been refactored
| * @throws Exception If an unexpected error occurs during the filter creation process. | ||
| * | ||
| */ | ||
| public void createFilter(Inspector2Client inspector2Client, String description) { |
There was a problem hiding this comment.
If this is creating a low severity filter, the name of the method should reflect that. Right now the name indicates you can create any filter.
| System.out.println(" • Generate detailed findings reports"); | ||
| System.out.println(" • Integrate with AWS Security Hub"); | ||
| waitForInputToContinue(scanner); | ||
|
|
There was a problem hiding this comment.
Needs a cleanup method that removes the example filters and gives the option to disable inspector.
| public void testScenario() { | ||
| assertDoesNotThrow(() -> { | ||
| int maxResults = 10; | ||
| inspectorActions.getAccountStatus(inspector); |
There was a problem hiding this comment.
This test doesn't run the scenario, it runs the actions only. Needs a true integration test that actually runs the scenario.
There was a problem hiding this comment.
Addressed this comment.
| Collections.synchronizedList(new ArrayList<>()); | ||
|
|
||
| CompletableFuture<String> future = new CompletableFuture<>(); | ||
| paginator.subscribe(new Subscriber<ListFindingsResponse>() { |
There was a problem hiding this comment.
The pagination in the latest updates looks way more complicated than necessary. It should follow the simplest pagination pattern, without page limits or overrides, like this:
`
List<JobSummary> jobSummaries = new ArrayList<>();
ListJobsPublisher listJobsPaginator = getAsyncClient().listJobsPaginator(listJobsRequest);
CompletableFuture<Void> future = listJobsPaginator.subscribe(response -> {
jobSummaries.addAll(response.jobSummaryList());
});
future.join();
return jobSummaries;
`
There was a problem hiding this comment.
For Inspector2 async paginators, you generally must use the reactive Subscriber pattern if you want fine-grained control
Other AWS SDKS -- like S3 async paginators, the SDK provides subscribe() that returns a CompletableFuture, so you can use the simpler pattern.
Inspector2 async paginator → must use reactive Subscriber
3 participants
This pull request adds Javas V2 Inspector examples
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.