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.

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

Open
wants to merge 30 commits into
base: master
Choose a base branch
from
Open

Conversation

wowselim
Copy link
Contributor

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.

@wowselim
Copy link
Contributor Author

wowselim commented Jun 17, 2021

@tsegismont this could use some cleanup but it already generates valid (as in it compiles & runs) projects. It currently fails however, if for example RxJava is added to the dependencies, since mutiny does not provide this artifact.
So I'm not sure if I should add some validation (like we already have for junit4 and junit5) or simply add special handling for artifacts not provided by mutiny.
A cleaner alternative would be to disable the UI elements that would cause invalid projects but I think that's out of scope for this PR.

Signed-off-by: Selim Dincer <wowselim@live.de>
@wowselim
Copy link
Contributor Author

I did a quick diff of the artifacts with mutiny and I found that the following are not included:

vertx-web-api-contract
vertx-web-templ-httl
vertx-web-sstore-cookie
vertx-web-sstore-redis
vertx-web-sstore-infinispan
vertx-rx-java
vertx-rx-java2
vertx-rx-java3
vertx-reactive-streams
vertx-sync
vertx-lang-kotlin-coroutines
vertx-amqp-bridge
vertx-jca
vertx-camel-bridge
vertx-dropwizard-metrics
vertx-health-check
vertx-zipkin
vertx-opentracing
vertx-opentelemetry
vertx-unit
vertx-hazelcast
vertx-infinispan
vertx-ignite
vertx-zookeeper
vertx-service-proxy
vertx-sockjs-service-proxy
vertx-grpc
vertx-service-factory
vertx-maven-service-factory
vertx-http-service-factory

So if a user selects any of these dependencies, they will be provided by vert.x instead of mutiny.

Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
@wowselim wowselim marked this pull request as ready for review June 20, 2021 15:22
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
@tsegismont
Copy link
Collaborator

@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

@jponge
Copy link
Member

jponge commented Jul 5, 2021

I haven't tested but it looks good on a first pass

Copy link
Collaborator

@tsegismont tsegismont left a 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' \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'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

Copy link
Contributor Author

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 \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
flavor==vert.x \

Same here

Copy link
Contributor Author

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";
Copy link
Collaborator

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.

Copy link
Contributor Author

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()) {
Copy link
Collaborator

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added in fa9cb0a

@@ -47,81 +48,98 @@
]
}
],
"stack": [
"vertxStack": [
Copy link
Collaborator

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.

Copy link
Contributor Author

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"
Copy link
Collaborator

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

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 <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
@wowselim
Copy link
Contributor Author

wowselim commented Jul 5, 2021

we need an update of the verticle and test file templates; when the Mutiny flavor is selected, the Mutiny API should be used

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 <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
@wowselim wowselim requested a review from tsegismont July 8, 2021 21:20
Signed-off-by: Selim Dincer <wowselim@live.de>
Signed-off-by: Selim Dincer <wowselim@live.de>
@GavinRay97
Copy link

This is awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add mutiny to the Reactive category in the dependencies
4 participants