| Great! In general, the main things that help a PR get merged:
- clearly stated goal
- minimal diff (no lines not directly contributing to goal)
- code structure making it obvious that existing scenarios would not regress
- convincing test coverage
Test coverage in this plugin is a hard problem, unfortunately. I run tests manually against S3 before release, and would run them against a tricky-looking PR before merge. (There used to be a private CI builder for this plugin with an AWS account, but that was shut down since it is not being actively developed: my employer is not currently prioritizing this over other things.) On the other hand, for this particular change there is an OSS provider which claims S3 compatibility—MinIO—so there may be a way to offer self-contained test coverage: use either Docker Fixtures or TestContainers to include a test suite with a Docker fixture for MinIO and delegate to ArtifactManagerTest.artifactStashAndDelete; see JCloudsArtifactManagerTest for the live AWS version. (I am assuming that S3BlobStore.toExternalURL actually works in MinIO. Various documentation suggests that it supports presigned URLs, but I have never tried it. See the plugin README for discussion of how presigned URLs are critical to the security model.) |
