Skip to content

Conversation

cescoffier
Copy link
Member

@cescoffier cescoffier commented Jan 29, 2019

@cescoffier cescoffier added this to the 0.8.0 milestone Jan 29, 2019
@cescoffier cescoffier requested a review from gsmet January 29, 2019 18:56
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Looks mostly good, added some comments inline.

hello shamrock!
----

TIP: If the application requires configuration values and these values are not set, an error is thrown.
Copy link
Member

Choose a reason for hiding this comment

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

A TIP admonition would make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

it's already an admonition (single line admonition).

Admonition blocks are supported too and allows adding title and paragraph as content. However, for short notes like this one I've found single-line more readable.


As usual, the application can be packaged using `mvn clean package` and executed using the `-runner.jar` file.
You can also generate the native executable.
You can also generate the native executable with `mvn clean package -Pnative`
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this patch but maybe we should be consistent with what we do in Shamrock proper and use -Dnative. We would need the profile to be activated on the property.

Not sure it's worth the work though. In any case, if we decide to do it, let's do it in a follow-up PR (and update all the guides).

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 tend to prefer specifying profiles explicitly, at least for the users. Now, if the user is a Maven user, he can definitely change that.

But we can change the behavior (it would require changing the template, doc, and quickstarts).

====
The events are also called in _dev mode_ between each redeployment.
====
TIP: The events are also called in _dev mode_ between each redeployment.
Copy link
Member

Choose a reason for hiding this comment

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

Oh, one line admonitions are supported this way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

@cescoffier cescoffier left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for the review. I've covered all the issue except one (the profile).
Let's track this one in a separate issue as it requires more coordination (doc, template, and quickstarts).

hello shamrock!
----

TIP: If the application requires configuration values and these values are not set, an error is thrown.
Copy link
Member Author

Choose a reason for hiding this comment

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

it's already an admonition (single line admonition).

Admonition blocks are supported too and allows adding title and paragraph as content. However, for short notes like this one I've found single-line more readable.


As usual, the application can be packaged using `mvn clean package` and executed using the `-runner.jar` file.
You can also generate the native executable.
You can also generate the native executable with `mvn clean package -Pnative`
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 tend to prefer specifying profiles explicitly, at least for the users. Now, if the user is a Maven user, he can definitely change that.

But we can change the behavior (it would require changing the template, doc, and quickstarts).

====
The events are also called in _dev mode_ between each redeployment.
====
TIP: The events are also called in _dev mode_ between each redeployment.
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Sorry, added 3 more comments after reviewing your changes. We're nearly there!


In your favorite IDE, create a new Maven project.
It should generate a `pom.xml` file with a content similar to:
The easiest way to create a new Protean project is to open a terminal and run the following command:
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, missed that on the first review, but Protean should be {project-name}. We use that consistently in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

Damn! Yes, thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

replaced everywhere.

Copy link
Member

Choose a reason for hiding this comment

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

Have you pushed your changes? It still appears as "Protean" on GitHub?

Copy link
Member Author

Choose a reason for hiding this comment

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

well... pushed on the wrong remote. Will cleanup once merged.

Copy link
Member Author

Choose a reason for hiding this comment

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

should be better now.

* use {project-name} instead of Protean
* rephrase the part on junit 5 and surefire
@cescoffier
Copy link
Member Author

I've fixed all of them! Thanks!

@cescoffier cescoffier merged commit 3249147 into quarkusio:master Jan 30, 2019
@cescoffier cescoffier deleted the features/various-documentation-fixes branch January 30, 2019 12:42
@cescoffier cescoffier added the kind/enhancement New feature or request label Jan 30, 2019
maxandersen added a commit to maxandersen/quarkus that referenced this pull request Nov 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert the getting started guide to use maven-shamrock-plugin:create

2 participants