-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
MongoDB with Panache quickstart #293
MongoDB with Panache quickstart #293
Conversation
Could you rename it to simply |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a couple of comments.
using-mongodb-panache/pom.xml
Outdated
<dependency> | ||
<groupId>io.quarkus</groupId> | ||
<artifactId>quarkus-mongodb-panache</artifactId> | ||
<version>999-SNAPSHOT</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That shouldn't be here. If it's generated with this then we have an issue with the bom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a chicken an egg issue, the quickstarts are written before the extension goes into Quarkus so we need to hardcode the version, almost all guides does this.
If I remove it, you will not be able to merge the quickstarts before 0.24 is published ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not an issue. We need the latest master deployed to test the quickstarts anyway. You can just remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8"> | ||
<title>using-mongodb-panache - 1.0-SNAPSHOT</title> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would drop the using-
from here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
using-mongodb-panache/pom.xml
Outdated
<configuration> | ||
<systemProperties> | ||
<native.image.path>${project.build.directory}/${project.build.finalName}-runner</native.image.path> | ||
</systemProperties> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will the tests run properly? I don't see anything to launch a MongoDB instance for the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no test in the quickstart
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah ah, silly me :).
de30c33
to
74d11ee
Compare
@gsmet I renammed to |
74d11ee
to
f19652b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good. We probably need to update the doc to reflect the artifact name/directory change.
Just marking it as non-mergeable as we need to wait for 0.23.0 to be released before merging this!
@gsmet, I'll update the doc. I'll send a PR in a few minutes :) |
It would be great to have a few tests. I tend to use the quickstart as a good sensor to know when we break things. |
@cescoffier as the quickstart uses the Quarkus current version (0.23.2 as of today), it didn't really works untill the current master is released. So I cannot add tests before 0.24. |
The PR must target the |
@cescoffier I just remember why I didn't include any test, if I keep the
So I need to use a non-snapshot version to build the test (like 0.23.2), when it is ready, switch back to the 999-SNAPSHOT version and commit ... I'll work on it later, I'll be on vacation for a week 🌞 |
OK, I'm merging this as we have the doc pointing to it. Let's add tests later. |
The issue is that it does not seem to work. |
The MongoDB with Panache quickstart application.