couchbase / CouchbaseMock

A Java mock for Couchbase
Apache License 2.0
64 stars 42 forks source link

Properly shade third party dependencies #33

Closed snicoll closed 5 years ago

snicoll commented 6 years ago

For some reason that I don't understand, this project repackages gson and a bunch of other libraries.

This prevents CouchbaseMock to be integrated in Spring Boot. If you decide to go with a one-jar only solution, please shade those libs so that they end up in a different package.

avsej commented 6 years ago

@snicoll you can always run CouchbaseMock in separate JVM, and this will eliminate all packaging issues. And this is how actually we run it.

If this does not fit your specific requirements, I would be happy to review your patch, which you can submit to our review system at https://review.couchbase.org

daschl commented 6 years ago

@snicoll is there a specific reason you want to get it into the boot pom? This project is not officially supported by couchbase at this point and might break or change down the line.

snicoll commented 6 years ago

@daschl there is a reference to the Spring Boot issue above your comment.

Thanks for sharing the scope of this project. I find it very confusing that a project that both share your groupId and repo is not officially supported. This and the clarification of how this project should be used confirms that we won't add support for it in Spring Boot.

@avsej there are users that are trying to embed CouchbaseMock in their app for testing purposes. IMO, that's a perfectly valid idea so if you don't support that, I think it should clearly say so.

avsej commented 6 years ago

I definitely support this idea. What I mean is that currently for our needs running mock in separate process serves well. And we always open for contributions, so if you have patch which is tested to work in your usecases, I will be glad to review and accept it.

daschl commented 6 years ago

it's valid that people use it for testing, we just don't officially support it. I think we should fix these errors as they come up and that people can use it in boot. I just wanted to add the note of caution here thats its different as like the couchbase SDK or our spring data integration.

snicoll commented 6 years ago

it's valid that people use it for testing, we just don't officially support it.

I am confused. What else could you do with a project called CouchbaseMock ?

daschl commented 6 years ago

sorry @snicoll I meant to say "external users" use it for testing (especially in the boot context), compared to we just use it internally.

avsej commented 6 years ago

Also we assume that users have to be familiar with CouchbaseMock internals or with the way how it works. As it is not drop-in replacement of Couchbase Server. For example it does not provide support for recent features like analytics or full-text search, also it does not have full support of N1QL. We might add it later, or can accept user contribution.

As domain and usecase of the problem described in this ticket on the Spring Boot side, is it possible that someone from user of from your team will fix, validate and send us the patch with the fix?

snicoll commented 6 years ago

is it possible that someone from user of from your team will fix, validate and send us the patch with the fix?

A fix for what? To clarify, we received a PR to add test slicing support for Couchbase. We do accept such option when there is a way to mock the underlying store. The external contributor decided to use CouchbaseMock (and that's really the only option available). In this issue, it became apparent that we can't and won't add support for CouchbaseMock in Spring Boot.

I am now processing that PR to see what I can add to make this feature available outside of Spring Boot in case the reporter wants to do that. I'll decline the PR for the reasons discussed above.

snicoll commented 6 years ago

As far as I am concerned, this issue can be closed if you clearly describe the scope of the library (i.e. that it shades dependencies and that it cannot be embedded).

avsej commented 6 years ago

I am now processing that PR to see what I can add to make this feature available outside of Spring Boot in case the reporter wants to do that. I'll decline the PR for the reasons discussed above.

Yes, that would be good. I definitely support you in directing reporters to contribute their fixes into CouchbaseMock first.

As far as I am concerned, this issue can be closed if you clearly describe the scope of the library (i.e. that it shades dependencies and that it cannot be embedded).

We are not against shades. Right now the only requirements is that final jar should be executable without any extra dependencies in classpath. So shaded libraries is an option.

snicoll commented 6 years ago

As far as this issue is concerned, I'd welcome a warning about not embedding CouchbaseMock in the project itself in the documentation.

Aloren commented 5 years ago

Hello! It would be nice to relocate CouchbaseMock dependencies to the different package than original as it leads to issues with classpath that can not be traced very easily. For example if gson is used in project and CouchbaseMock is added in test scope there might be a chance of having Gson classes on classpath from CouchbaseMock. And since CouchbaseMock uses quite old version, you may get NoSuchMethodError. @avsej is it possible for CouchbaseMock to use class relocation for its dependecies?

avsej commented 5 years ago

@Aloren I would be happy to see and review the patch. But so far we've successfully used the mock in separate process across our projects. I belive relocating deps will not hurt.

Aloren commented 5 years ago

@avsej ok. i will create pr. thanks!

avsej commented 5 years ago

@Aloren note that we use http://review.couchbase.org/ for code submissions and review, and generally do not merge PRs. If you want your change accepted, you have to register on gerrit, sign CLA and submit your patch there.

avsej commented 5 years ago

@Aloren I've left notes in http://review.couchbase.org/c/102748/, please let me know there if you have any questions about it.

Aloren commented 5 years ago

ok, i will check that in the evening. thanks for the review

avsej commented 5 years ago

Fixed in 99084d4fab780e1abc1ce4cab4940c6824f3d343