Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to specify custom endpoint url #138

Merged
merged 6 commits into from
Oct 15, 2021

Conversation

rsandell
Copy link
Member

@rsandell rsandell commented Oct 7, 2021

You can specify the system property hudson.plugins.s3.ENDPOINT or environment variable PLUGIN_S3_ENDPOINT to a custom http(s) url. When the custom endpoint is set the Region setting will have no effect.

Change to non deprecated API
Modernize the plugin pom
Added realistic tests using the custom endpoint

Change to non deprecated API
Modernise the plugin pom
Added realistic tests
Copy link
Member

@jtnord jtnord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main issue is the ENDPOINT envvar - should this use SystemProperties? (is it run on the agent or the controller), but in any case the getenv calls looks like it will generally fail

Other than that looks OK to me

src/main/java/hudson/plugins/s3/ClientHelper.java Outdated Show resolved Hide resolved
src/main/java/hudson/plugins/s3/ClientHelper.java Outdated Show resolved Hide resolved
src/main/java/hudson/plugins/s3/S3CopyArtifact.java Outdated Show resolved Hide resolved
src/main/java/hudson/plugins/s3/ClientHelper.java Outdated Show resolved Hide resolved
src/main/java/hudson/plugins/s3/S3ArtifactsAction.java Outdated Show resolved Hide resolved
src/test/java/hudson/plugins/s3/MiniIOTest.java Outdated Show resolved Hide resolved
src/test/java/hudson/plugins/s3/MiniIOTest.java Outdated Show resolved Hide resolved
src/test/java/hudson/plugins/s3/MiniIOTest.java Outdated Show resolved Hide resolved
@rsandell rsandell requested review from jtnord and jglick October 8, 2021 08:52
Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Manual configuration of the endpoint in agent VMs seems unnecessary.

@rsandell
Copy link
Member Author

I have finally managed to verify that the new code works against S3 as before. So now I can continue fixing the code review comments.

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable.

@@ -24,7 +24,11 @@
com.amazonaws.services.s3.model.Region.US_Standard.toAWSRegion().getName());
public static final String ENDPOINT = System.getProperty("hudson.plugins.s3.ENDPOINT", System.getenv("PLUGIN_S3_ENDPOINT"));

public static AmazonS3 createClient(String accessKey, String secretKey, boolean useRole, String region, ProxyConfiguration proxy)
public static AmazonS3 createClient(String accessKey, String secretKey, boolean useRole, String region, ProxyConfiguration proxy) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did that at first, but as I was editing various calls to it, it felt like a good shortcut to have.

@rsandell rsandell requested a review from daniel-beck October 14, 2021 07:55
Copy link
Member

@alecharp alecharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@pecastro
Copy link

README update ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants