NIFI-14384 Adds Force Path Style support to S3 Bundle Persistence Pro… #10331
Conversation
…vider - Added FORCE_PATH_STYLE configuration property with default false - Enhanced S3BundlePersistenceProvider to support path-style URLs for S3-compatible storage - Added comprehensive integration tests with MinIO support - Updated documentation with setup instructions and environment variables The implementation adds support for Force Path Style addressing, enabling compatibility with S3-compatible object storage systems like MinIO, Ceph, and other non-AWS implementations. The property defaults to false to maintain backward compatibility with AWS S3. The implementation includes: - New FORCE_PATH_STYLE property with proper default handling - Environment variable NIFI_REGISTRY_S3_FORCE_PATH_STYLE support - Comprehensive integration tests with MinIO deployment instructions - Updated documentation for all configuration properties The changes are backward compatible and don't affect existing AWS S3 deployments.
There was a problem hiding this comment.
Thanks for proposing this improvement @shubalex. The general change looks straightforward, I noted one implementation concern on the integration test, but otherwise this looks good.
| } | ||
| } | ||
|
|
||
| private String getKeyViaReflection(BundleVersionCoordinate coordinate) { |
There was a problem hiding this comment.
Using reflection and overriding visibility is generally poor practice for tests, as it makes the test more brittle, and tightens the coupling between the test and the implementation class. I recommend removing this method and either simplifying the test method or evaluating other available output from public methods.
There was a problem hiding this comment.
@shubalex Any updates on this? I would be for moving this forward without introducing the complex changes to the integration test.
|
@exceptionfactory Hello! Thank you for your feedback. I can fix the tests within a few days. Alternatively, you can accept the main functionality, and then I can fix the tests with a separate MR, if it's possible. What would be the best approach? |
Thanks for the reply @shubalex. Given the narrow and optional nature of the new configuration property, I'm open to removing the test from this pull request, and considering the test in a separate pull request. |
|
Great! Then I'll be back in a few days with the tests. |
Thanks @shubalex. Just to clarify, are you planning to revert the updates to the integration test, and follow up on that separately? I'm happy to merge the PR if you can remove the integration test for now. If you mean to follow up on this PR with test changes, I can wait for that, either way. |
3 participants
Summary
NIFI-14384 Adds Force Path Style support to S3 Bundle Persistence Provider
The implementation adds support for Force Path Style addressing, enabling compatibility with S3-compatible object storage systems like MinIO, Ceph, and other non-AWS implementations. The property defaults to false to maintain backward compatibility with AWS S3.
The implementation includes:
The changes are backward compatible and don't affect existing AWS S3 deployments.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-14384NIFI-14384Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation