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.

Sign up for GitHub

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

Jump to bottom

Panache REST Data with MongoDB #12158

Merged
merged 1 commit into from
Oct 14, 2020
Merged

Panache REST Data with MongoDB #12158

merged 1 commit into from
Oct 14, 2020

Conversation

gytis
Copy link

@gytis gytis commented Sep 17, 2020

Fixes #8700

@geoand geoand requested a review from loicmathieu September 17, 2020 10:19
loicmathieu
loicmathieu requested changes Sep 21, 2020
View reviewed changes
Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Some remarks / questions

...panache/mongodb-rest-data-panache/runtime/src/main/resources/META-INF/quarkus-extension.yaml Show resolved Hide resolved
...-rest-data-panache/src/main/java/io/quarkus/it/mongodb/rest/data/panache/TestController.java Outdated Show resolved Hide resolved
extensions/panache/mongodb-rest-data-panache/deployment/pom.xml Outdated Show resolved Hide resolved
@gytis
Copy link
Author

gytis commented Oct 5, 2020

@loicmathieu is there something else missing in this PR?

loicmathieu
loicmathieu approved these changes Oct 5, 2020
View reviewed changes
Copy link
Contributor

@loicmathieu loicmathieu left a comment

Choose a reason for hiding this comment

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

LGTM

@loicmathieu
Copy link
Contributor

@gytis it's OK for me, great PR, thanks!

As it's a big PR with a new extension I'd prefere having a review for someone else also. Maybe @geoand or @FroMage can have a look ?

@loicmathieu loicmathieu requested review from geoand and FroMage October 7, 2020 12:57
FroMage
FroMage approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@FroMage FroMage left a comment

Choose a reason for hiding this comment

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

This looks fine, it's a copy of the other one and I know this will Just Work™.

How does resteasy-links generate the links for you, given that it depends on JPA @Id annotations IIRC and Mongo is not using those?

@FroMage
Copy link
Member

FroMage commented Oct 7, 2020

And I must apologise for my late review.

@gytis
Copy link
Author

gytis commented Oct 7, 2020

@FroMage I add a ResourceID annotation on build time https://github.com/quarkusio/quarkus/pull/12158/files#diff-fc2a66e3fd296d54f0d8c744ab6d90afR115.

However, I've heard a rumour that you're working on Quarkus REST. So maybe once you're done we could add some alternative to RESTEasy link and avoid such hack.

@gytis
Copy link
Author

gytis commented Oct 14, 2020

@geoand shall we merge?

@geoand
Copy link
Contributor

geoand commented Oct 14, 2020

Ah, I just saw @FroMage approved, so yes

@geoand geoand merged commit 815e1fc into quarkusio:master Oct 14, 2020
@geoand geoand added this to the 1.10 - master milestone Oct 14, 2020
@gytis gytis deleted the rest-data-mongo branch October 14, 2020 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@loicmathieu loicmathieu loicmathieu approved these changes

@FroMage FroMage FroMage approved these changes

@geoand geoand Awaiting requested review from geoand

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.10.0.CR1
Development

Successfully merging this pull request may close these issues.

Implements Panache REST for MongoDB
4 participants
@gytis @loicmathieu @FroMage @geoand