feat: update S3 bucket name in configuration and add logging for buck…#36
feat: update S3 bucket name in configuration and add logging for buck…#36
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR updates the default S3 bucket fallback in the application configuration and adds logging of existing S3 buckets on application startup.
- Added a default fallback value for
AWS_S3_BUCKET_NAMEinapplication.yaml - Introduced a
@PostConstructmethod inAwsConfigto list and log S3 buckets at startup
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/main/resources/application.yaml | Default S3 bucket name fallback set to oceancurrent-testing |
| src/main/java/au/org/aodn/oceancurrent/configuration/aws/AwsConfig.java | Added runTestToListS3Buckets startup method for bucket listing |
Comments suppressed due to low confidence (2)
src/main/java/au/org/aodn/oceancurrent/configuration/aws/AwsConfig.java:67
- [nitpick] The method name
runTestToListS3Bucketsis misleading since this code executes in production; consider renaming it tolistS3BucketsOnStartupor clarifying its purpose.
private void runTestToListS3Buckets() {
src/main/resources/application.yaml:44
- [nitpick] The new default bucket name
oceancurrent-testingshould be documented in the project’s configuration guide or README so all developers understand the fallback behavior.
bucket-name: ${AWS_S3_BUCKET_NAME:oceancurrent-testing}
| @PostConstruct | ||
| private void runTestToListS3Buckets() { |
There was a problem hiding this comment.
Using @PostConstruct to run an S3 bucket listing at startup can introduce unnecessary network calls and slow application initialization in production; consider moving this into a dedicated health endpoint or enabling it only under a non-production profile.
| @PostConstruct | |
| private void runTestToListS3Buckets() { | |
| public void listS3BucketsForHealthCheck() { |
|
|
||
| @PostConstruct | ||
| private void runTestToListS3Buckets() { | ||
| try (S3Client s3Client = s3Client()) { |
There was a problem hiding this comment.
Instantiating a new S3Client instead of injecting the existing bean bypasses Spring’s lifecycle and configuration; inject the S3Client bean to leverage singleton scope and improve testability.
| } catch (Exception e) { | ||
| log.error("Unexpected error during S3 bucket listing:", e); |
There was a problem hiding this comment.
Catching a generic Exception can mask unexpected errors; narrow this catch block to specific exception types or rethrow unhandled exceptions to ensure issues aren’t silently ignored.
| } catch (Exception e) { | |
| log.error("Unexpected error during S3 bucket listing:", e); | |
| } catch (RuntimeException e) { | |
| log.error("Unexpected runtime error during S3 bucket listing:", e); | |
| throw e; // Rethrow to avoid masking critical runtime exceptions | |
| } catch (Exception e) { | |
| log.error("Unexpected checked exception during S3 bucket listing:", e); | |
| throw new RuntimeException("Unhandled exception during S3 bucket listing", e); // Wrap and rethrow |
…et listing