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

STORM-3242: Adds "examples" and "externals" profiles #2858

Merged

Conversation

d2r
Copy link

@d2r d2r commented Oct 2, 2018

The existing modules have been moved into like-named Maven profiles that
are enabled by default.

They can be disabled, for example, with:

mvn clean install -DskipTests=true -P '!examples,!externals'

Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

I am +1 on the concept, but a little nervous that how we are doing the travis build might result in some issues.

@d2r
Copy link
Author

d2r commented Oct 2, 2018

Agree. If there are errors/failures in travis-ci, then I will take a look.

Copy link
Contributor

@kishorvpatil kishorvpatil left a comment

Choose a reason for hiding this comment

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

👍 LGTM. This would help a lot during testing. Thank you @d2r

@srdo
Copy link
Contributor

srdo commented Oct 3, 2018

This looks good, but I'm wondering if Flux and the sql modules should be added to externals as well? They depend on storm-kafka-client, so the build won't work with externals disabled unless you happen to already have storm-kafka-client in your local repo.

Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

+1

@revans2
Copy link
Contributor

revans2 commented Oct 3, 2018

Sorry @srdo is right we need to make sure that we have the dependencies setup properly.

@d2r
Copy link
Author

d2r commented Oct 3, 2018

Yes, thank you @srdo for pointing this out.

Would it be all right merely to move flux and sql to the new externals profile, or would we need make source code/layout changes as well to keep things organized?

@srdo
Copy link
Contributor

srdo commented Oct 3, 2018

I'd be fine with just moving them to the new profile. It already contains the integration test, which also isn't in externals.

@d2r
Copy link
Author

d2r commented Oct 3, 2018

OK this is done.
Tested using:

rm -rf ~/.m2/repository/
mvn clean install -DskipTests=true -U -P '!examples,!externals'

@srdo
Copy link
Contributor

srdo commented Oct 4, 2018

+1, thanks for addressing my comment. Please squash to one commit containing the issue number.

The existing modules have been moved into like-named Maven profiles that are
enabled by default.

Updates travis-ci script to use new profiles

https://maven.apache.org/guides/introduction/introduction-to-profiles.html

> All profiles that are active by default are automatically deactivated
> when a profile in the POM is activated on the command line or through
> its activation config.

Therefore we should activate the new profiles whenever we explicitly
activate another profile.

Moves sql and flux to externals profile
@d2r d2r force-pushed the storm-3242-examples-externals-maven-profiles branch from 9aeac5e to a9ff440 Compare October 4, 2018 14:30
@d2r
Copy link
Author

d2r commented Oct 4, 2018

Squashed; old commit was 9aeac5e.

@srdo
Copy link
Contributor

srdo commented Oct 4, 2018

Thanks @d2r, merged to master.

@asfgit asfgit merged commit a9ff440 into apache:master Oct 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants