Skip to content

Conversation

@kensipe
Copy link
Member

@kensipe kensipe commented Jun 10, 2015

large change to build as a multi-project build. It was too much work to rebase the previous PR-77

This is basically the same work however I've updated all the docs now and have tested the vagrant configuration.

Lets get this checked and merged!

@kensipe
Copy link
Member Author

kensipe commented Jun 10, 2015

@DarinJ @mohitsoni @adam-mesos @smarella any review here. I would like to merge this, this week. Tomorrow if possible.

@DarinJ
Copy link

DarinJ commented Jun 10, 2015

Should we distribute: gradle/wrapper/gradle-wrapper.jar?

@kensipe
Copy link
Member Author

kensipe commented Jun 10, 2015

no... that is not part of the distro... only used for the build process

@kensipe
Copy link
Member Author

kensipe commented Jun 10, 2015

several people seem confused by the gradle-wrapper.jar and the fact that we have a binary checked into the repo. It is idiomatic gradle code to include it. it is a 25k jar that is a bootstrap jar for pulling the correct version of gradle for the build. It ensures that builds always work because the meta-data for what the correct tool and version for building is included with the project.

Copy link

Choose a reason for hiding this comment

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

probably should be consistant with myriad-executor-runnable-0.0.1.jar and myriad-executor-runnable-xxx.jar throughout documentation. That's not on you though.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok... I've updated several. here is the rule I followed: 1) if it was documentation / mentioned in docs, it has myriad-executor-runnable-x.x.x.jar, however if 2) it was used in a config file example like yml or part of a shell example I left the myriad-executor-runnable-0.0.1.jar version.

@DarinJ
Copy link

DarinJ commented Jun 10, 2015

During build two warnings about style. +1 for warnings instead of errors, but would be nice if Travis would still complain.
First look no deal breakers, one comment on style. Will look once more tomorrow morning.

@kensipe
Copy link
Member Author

kensipe commented Jun 11, 2015

regarding warnings. I was trying to get this PR in with min changes to the project... just the structure. however I will take a look and fix these.

@kensipe
Copy link
Member Author

kensipe commented Jun 11, 2015

done! @DarinJ what do you think... are we good to merge ?

@pdread100
Copy link

+1

@DarinJ
Copy link

DarinJ commented Jun 11, 2015

@kensipe if you've tested it, I think we're good. I got through the build on a cluster last night, but haven't had a chance to install. I saw no other issues today.

@kensipe
Copy link
Member Author

kensipe commented Jun 11, 2015

I did some vagrant testing... I think we are good to go!

kensipe added a commit that referenced this pull request Jun 11, 2015
@kensipe kensipe merged commit 6940319 into phase1 Jun 11, 2015
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