-
Notifications
You must be signed in to change notification settings - Fork 79
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 plugin examples to Styx #432
Conversation
plugin-examples/pom.xml
Outdated
@@ -1,205 +1,82 @@ | |||
<?xml version="1.0" encoding="UTF-8"?> | |||
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" | |||
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/maven-v4_0_0.xsd"> | |||
<parent> |
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.
If we add the parent as a dependency, all the parent pom's dependencies will be transitive dependencies of the plugin. This means that if we wanted to shade it with all its dependencies in a later stage we would re-add (with a different package name) many of Styx dependencies we don't really need...
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 would probably recommend using composition (i.e., importing a pom) here. In any case, it would be great if you could verify that if we try to use the add the plugin to Styx it's easy to include the required dependencies only.
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 change was added to ensure that the plugin examples build properly as part of the Styx project. I will see if any alternative is possible.
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.
See my PR comments for requested changes.
plugin-examples/pom.xml
Outdated
</dependency> | ||
<dependency> | ||
<groupId>io.reactivex</groupId> | ||
<artifactId>rxjava</artifactId> | ||
<version>${rxjava.version}</version> | ||
<scope>provided</scope> |
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 would keep the provided scope as this is still not needed in our "plugin bundle"
plugin-examples/pom.xml
Outdated
<dependencies> | ||
<dependency> | ||
<groupId>com.hotels.styx</groupId> | ||
<artifactId>styx-api</artifactId> | ||
<version>${styx.version}</version> | ||
<!-- The dependency is explicitlu needed only during compile time, styx is already available in the runtime--> | ||
<scope>provided</scope> |
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 would still keep the provided scope as we don't need to package/bundle the api with our plugin.
This PR adds some plugin example classes.