-
Notifications
You must be signed in to change notification settings - Fork 40
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
Add mutiny support #189
base: master
Are you sure you want to change the base?
Add mutiny support #189
Conversation
update fork
Signed-off-by: Selim Dincer <[email protected]>
@tsegismont this could use some cleanup but it already generates valid (as in it compiles & runs) projects. |
Signed-off-by: Selim Dincer <[email protected]>
I did a quick diff of the artifacts with mutiny and I found that the following are not included:
So if a user selects any of these dependencies, they will be provided by vert.x instead of mutiny. |
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
@wowselim sorry for the delay, we're working on getting 4.1.1 out, I haven't been able to find some time to review yet |
I haven't tested but it looks good on a first pass |
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.
Thank you @wowselim and sorry again for the delay.
In addition to the inline comments:
- we need an update of the verticle and test file templates; when the Mutiny flavor is selected, the Mutiny API should be used
- as much as I like the flavor wording in discussion, I would prefer not to use it on the website; how about Vert.x API?
README.md
Outdated
|
||
Full example: | ||
|
||
``` | ||
curl -X GET \ | ||
'http://start.vertx.io/starter.zip?artifactId=starter&buildTool=maven&groupId=io.vertx&language=java&vertxDependencies=&vertxVersion=4.1.1' \ | ||
'http://start.vertx.io/starter.zip?artifactId=starter&buildTool=maven&groupId=io.vertx&language=java&vertxDependencies=&vertxVersion=4.1.1&flavor=vert.x' \ |
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.
'http://start.vertx.io/starter.zip?artifactId=starter&buildTool=maven&groupId=io.vertx&language=java&vertxDependencies=&vertxVersion=4.1.1&flavor=vert.x' \ | |
'https://start.vertx.io/starter.zip?artifactId=starter&buildTool=maven&groupId=io.vertx&language=java&vertxDependencies=&vertxVersion=4.1.1' \ |
Let's keep the default for simple examples
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.
removed in bdd8041
README.md
Outdated
@@ -51,6 +52,7 @@ artitfactId==starter \ | |||
language==java \ | |||
buildTool==maven \ | |||
vertxVersion==4.1.1 \ | |||
flavor==vert.x \ |
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.
flavor==vert.x \ |
Same here
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.
removed in bdd8041
@@ -29,4 +29,7 @@ | |||
String ARCHIVE_FORMAT = "archiveFormat"; | |||
String PACKAGE_NAME = "packageName"; | |||
String JDK_VERSION = "jdkVersion"; | |||
String FLAVOR = "flavor"; | |||
String MUTINY_FLAVOR = "mutiny"; |
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 don't believe this is necessary. It can be stored as an enum value.
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.
addressed in e098a10
List<Set<Dependency>> testDeps = Arrays.asList( | ||
Collections.singleton(new Dependency().setArtifactId("vertx-unit")), | ||
Collections.singleton(new Dependency().setArtifactId("vertx-junit5")) | ||
); | ||
|
||
Stream.Builder<VertxProject> builder = Stream.builder(); | ||
for (BuildTool buildTool : BuildTool.values()) { |
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 should test both flavors here.
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.
added in fa9cb0a
src/main/resources/starter.json
Outdated
@@ -47,81 +48,98 @@ | |||
] | |||
} | |||
], | |||
"stack": [ | |||
"vertxStack": [ |
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 will be simpler to maintain this file if we:
- keep a single stack definition
- add a
mutinyBindings
boolean attribute to items
The attribute value would indicate if the module has Mutiny bindings.
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 in 03b4d28.
I felt pretty dirty creating that abomination of a json document initially but I thought it would be nicer than adding the mutiny prefix in the java code later. In hindsight, I like your approach a lot more. Much cleaner 👍
<label class="col-sm-4 col-form-label">Flavor</label> | ||
<div class="col-sm-8"> | ||
<div class="btn-group"> | ||
<label class="btn btn-sm btn-default" ng-repeat="flavor in vm.flavors" |
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 need some logic to disable dependencies that don't have Mutiny bindings when the Mutiny flavor is selected.
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.
Is there a practical reason why it shouldn't be allowed? What happens if a user wants to create a project with the mutiny flavor but wants to use the zipkin artifact 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.
I added a similar message to that of the vertx-unit / junit5 combination in dc01e0c. WDYT?
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
I can do this with freemarker conditionals or create new templates for the mutiny flavor. What do you think makes more sense @tsegismont? And now I need to learn how to test with mutiny, huh? Great opportunity 😄 |
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
Signed-off-by: Selim Dincer <[email protected]>
This is awesome! |
Motivation:
closes #188
This PR adds support for a mutiny project "Flavor". Since mutiny differs from other dependencies in that it provides a whole new set of artifacts, I decided to make this a flavor rather than a dependency.