-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
App config parameter support for project creation #16476
App config parameter support for project creation #16476
Conversation
Thanks for your pull request! The title of your pull request does not follow our editorial rules. Could you have a look?
|
devtools/maven/src/main/java/io/quarkus/maven/CreateProjectMojo.java
Outdated
Show resolved
Hide resolved
Forgot to mentioned: In case of conflicts with other properties coming from codestarts,
Forgot to mentioned. In case of conflicts with other properties coming from codestarts, the codestarts ones will stay so I don't know if I just log these conflicts. |
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.
Thanks this looks great! beside a few suggestions..
devtools/maven/src/main/java/io/quarkus/maven/CreateProjectMojo.java
Outdated
Show resolved
Hide resolved
.../quarkus/devtools/codestarts/core/strategy/SmartConfigMergeCodestartFileStrategyHandler.java
Outdated
Show resolved
Hide resolved
...ools/devtools-common/src/main/java/io/quarkus/devtools/project/codegen/ProjectGenerator.java
Outdated
Show resolved
Hide resolved
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 3662319
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 integration-tests/kotlin✖ 📦 integration-tests/scala✖ ⚙️ JVM Tests - JDK 11 Windows #📦 integration-tests/kotlin✖ 📦 integration-tests/scala✖ ⚙️ JVM Tests - JDK 15 #📦 integration-tests/kotlin✖ 📦 integration-tests/scala✖ ⚙️ JVM Tests - JDK 8 #📦 integration-tests/kafka✖ ✖ 📦 integration-tests/kotlin✖ 📦 integration-tests/scala✖ ⚙️ Maven Tests - JDK 11 #📦 integration-tests/maven✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ⚙️ Maven Tests - JDK 11 Windows #📦 integration-tests/maven✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ ✖ |
...ndent-projects/tools/devtools-common/src/main/java/io/quarkus/platform/tools/ToolsUtils.java
Outdated
Show resolved
Hide resolved
.../quarkus/devtools/codestarts/core/strategy/SmartConfigMergeCodestartFileStrategyHandler.java
Outdated
Show resolved
Hide resolved
...ndent-projects/tools/devtools-common/src/main/java/io/quarkus/platform/tools/ToolsUtils.java
Outdated
Show resolved
Hide resolved
...ndent-projects/tools/devtools-common/src/main/java/io/quarkus/platform/tools/ToolsUtils.java
Outdated
Show resolved
Hide resolved
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 62264f0
|
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building b20dbfa
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 devtools/cli✖ ⚙️ JVM Tests - JDK 11 Windows #📦 devtools/cli✖ ⚙️ JVM Tests - JDK 15 #📦 devtools/cli✖ |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building 9b798a5
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 devtools/cli✖ ⚙️ JVM Tests - JDK 11 Windows #📦 devtools/cli✖ ⚙️ JVM Tests - JDK 15 #📦 devtools/cli✖ |
Could you squash it one commit please :) |
This workflow status is outdated as a new workflow run has been triggered. Failing Jobs - Building ecb033b
Full information is available in the Build summary check run. Test Failures⚙️ JVM Tests - JDK 11 #📦 devtools/cli✖ 📦 extensions/jdbc/jdbc-mysql/deployment✖ ⚙️ JVM Tests - JDK 11 Windows #📦 devtools/cli✖ ⚙️ JVM Tests - JDK 15 #📦 devtools/cli✖ |
@cristian-com could you squash to one commit and rebase from main please |
5e3eda7
to
17101b6
Compare
@ia3andy Done, I’ll check the rebase is right |
…ation.yml/properties when creating a project either using maven plugin or cli.
21e3fd5
to
6102113
Compare
I am giving it a try while the CI is running.. Very nice PR and good feature @cristian-com thanks!!! |
Ok it works very well and it is pretty cool!
EDIT: it's already there, so never mind thanks.. |
Nice, thanks for checking. |
Failing Jobs - Building 6102113
Full information is available in the Build summary check run. Test Failures⚙️ MicroProfile TCKs Tests #📦 tcks/microprofile-rest-client-reactive✖ |
@ia3andy there is one test failing, I'm guessing it is not related, it seems like some kind of timeout. I can not see an option to re-run only that job. could you suggest how to proceed then ? |
Great Job! |
ok. I'm reorganizing the cli.. quite a bit for 2.x (as Andy is aware, see #16407 .. ) |
Hey, the original issue is for the maven plugin actually. Based on this comment I also added to CLI #12924 (comment) In my opinion, I see it quite handy when you want to add one-two properties when creating a new project, for more than that its just better to add the props directly to the file after created. |
@ebullient this was a feature request by the community. I think it's really nice to be able to have the config ready when generating an app. The config is really powerful in Quarkus and is key to many of our extensions. |
Right. As noted above, the feature requested by the community was for maven (and backend things), and I could see that being used inside a bazillion other tools, so I have no issue w/ that. It is specifically CLI usage that I'm asking about (which was an amendment/suggestion by Max, and not the original request). |
@ebullient I am not sure if this has changed, but if I remember well, we should as much as possible have the same features in CLI and Maven plugin (cc @maxandersen). Creating a project using Maven or CLI should be as similar as possible IMHO. |
It's a matter of option flood / recommended practices. The CLI and maven/gradle build tools will/may drift based on capability/usability requirements (CLI does not need to / should not always emulate what maven/gradle do. I'm not saying they will drift hugely, but I am leaving room for change, e.g. buildless). |
Fixes #12924
Draft commit for supporting a new parameter to set config properties that will end up in the application.yml/properties when creating a project either using maven plugin or cli.
Hi there,
Since this is my first fix I just want to make sure someone have a look before going any further. A couple of questions/notes:
I'm going to add the same property for CLI, just would be good if you check the property name "configProperties".
I'm updating documentation because there are some broken links (I'm guessing because of refactoring), I will also document the new parameter.
I'm creating the constant
APP_CONFIG_PROPERTIES
in bothSmartConfigMergeCodestartFileStrategyHandler.java
&ProjectGenerator.java
. I can not see a common file where this definition makes more sense.Since there is not support for maps in command line, I'm assuming a string with the below format should be enough.
1- Should I add other parameter to change the property and key-value separator?
2- Splitter is marked as @beta, I can change it if that is an issue
3- I've done some testing with codestarts adding various extensions, I've seen that when writing the .properties there is not a specific order so properties that should be together are not. e.g
Should I open a new issue for this ? or can I just fix it ?