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

Allow clients to customize s3mock server TLS certificate #231

Merged
merged 2 commits into from
Aug 11, 2020

Conversation

vlsi
Copy link
Contributor

@vlsi vlsi commented Jul 21, 2020

This enables to use of a regular S3 client with full TLS verification.

Related Issue

#230

Tasks

  • I have signed the CLA.
  • I have written tests and verified that they fail without my change.

@vlsi vlsi force-pushed the ssl_props branch 2 times, most recently from fdb7e3d to d2fe467 Compare July 22, 2020 09:23
@vlsi vlsi marked this pull request as ready for review July 22, 2020 09:23
@vlsi
Copy link
Contributor Author

vlsi commented Jul 22, 2020

@agudian , would you please review?

@vlsi vlsi force-pushed the ssl_props branch 7 times, most recently from 29456f0 to 2f80532 Compare July 22, 2020 14:16
This enables to use a regular S3 client with full TLS verification
@vlsi
Copy link
Contributor Author

vlsi commented Jul 22, 2020

@agudian , I'm sorry I give up.

I can't build the project locally (see #235 ), and the build error messages like the following are completely insane.

[WARNING] Missing header in: /home/travis/build/adobe/S3Mock/testsupport/junit5/src/test/java/com/adobe/testing/s3mock/junit5/sdk1/CustomCertificateTest.java

I do understand the necessity of the legal stuff, however, ill error messages are completely demotivating, and I don't want to spend my time fighting with Checkstyle / Maven.
It looks like I've spent half a day adding a trivial getters + test to the project, and even now Travis build fails. I will no longer update the PR. I don't think I want to contribute more taking into account the barrier is so high :-/

It would be great if you could remove license-maven-plugin / Checkstyle from your build completely.
There are modern tools that provide human-readable error messages and even automatic error correction: https://github.com/diffplug/spotless, https://github.com/autostyle/autostyle, https://github.com/pinterest/ktlint

Please feel free to reject my PRs.

Have a nice day.

@agudian
Copy link
Member

agudian commented Aug 11, 2020

@vlsi So sorry to hear that, fighting with a bad developer experience like you did is always a bummer and I totally get it. That building locally doesn't work for you is bad enough, and that the checkstyle warnings don't help is similarly bad. Add to that the lack of response from me (was on vacation). So I understand your frustration, and thanks for your effort anyway.

If you still have a need for your contributions here, let me know and I'll fix up the PR (I think the license header should contain 2017-2020 instead of 2020, at least according to the checkstyle rule).

@vlsi
Copy link
Contributor Author

vlsi commented Aug 11, 2020

was on vacation

Vacation is great.

I think the license header should contain 2017-2020 instead of 2020, at least according to the checkstyle rule

Well, I can try adding 2017-2020, however, it does sound weird. Technically speaking, CustomCertificateTest.java was created in 2020, so adding a copyright claim since 2017 is not right. The file did not exist in 2017. It would be a false copyright claim.

@vlsi
Copy link
Contributor Author

vlsi commented Aug 11, 2020

Ok, 2017-2020 pleases Checkstyle for the newly added file, however, technically speaking the proper header should be just 2020.

@agudian agudian merged commit 1598b7c into adobe:master Aug 11, 2020
@agudian
Copy link
Member

agudian commented Aug 11, 2020

lawyers, right? 🤷

Thanks a lot! I'll add you to the contributors list and run a release build.

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

Successfully merging this pull request may close these issues.

2 participants