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 plugin examples to Styx #432

Merged
merged 11 commits into from
Jul 1, 2019
Merged

Add plugin examples to Styx #432

merged 11 commits into from
Jul 1, 2019

Conversation

kvosper
Copy link
Contributor

@kvosper kvosper commented Jun 12, 2019

This PR adds some plugin example classes.

@kvosper kvosper requested review from mikkokar and dvlato and removed request for mikkokar June 12, 2019 13:30
@kvosper kvosper mentioned this pull request Jun 12, 2019
@@ -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>
Copy link
Contributor

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...

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@dvlato dvlato left a 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.

</dependency>
<dependency>
<groupId>io.reactivex</groupId>
<artifactId>rxjava</artifactId>
<version>${rxjava.version}</version>
<scope>provided</scope>
Copy link
Contributor

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"

<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>
Copy link
Contributor

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.

@kvosper kvosper merged commit d260cb6 into ExpediaGroup:master Jul 1, 2019
@kvosper kvosper deleted the pluginexample branch July 1, 2019 16:01
dvlato pushed a commit that referenced this pull request Jul 3, 2019
Add plugin examples to Styx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants