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

Support passing toolHome to Maven/Buck/Bazel invoker #252

Merged
merged 7 commits into from
Sep 18, 2020

Conversation

blindpirate
Copy link
Contributor

@blindpirate blindpirate commented Sep 11, 2020

So that they can be invoked programmatically more easily.

Fixes #245

@blindpirate blindpirate self-assigned this Sep 11, 2020
@blindpirate blindpirate force-pushed the blindpirate/tool-home branch 2 times, most recently from ade0fdc to dee7b20 Compare September 14, 2020 04:52
Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

I think we should add a separate base class for the (Maven|Bazel)ScenarioDefinition and the (Maven|Bazel)ScenarioInvoker. Then we can move the shared code from ScenarioInvoker there, instead of leaving the code in ScenarioInvoker and not using it in GradleScenarioInvoker.

@wolfs
Copy link
Member

wolfs commented Sep 14, 2020

@blindpirate There are quite some changes regarding the invokers going on in #253, I think we should hold off with this PR until #253 has been merged.

@wolfs wolfs changed the title Support pass toolHome to Maven/Buck/Bazel invoker Support passing toolHome to Maven/Buck/Bazel invoker Sep 14, 2020
@wolfs
Copy link
Member

wolfs commented Sep 16, 2020

@blindpirate #253 has been merged, so we can go forward with this change.

So that they can be invoked programmatically more easily.
Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

Nice work! I left two more comments.

Copy link
Member

@wolfs wolfs left a comment

Choose a reason for hiding this comment

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

LGTM! Can you add the new scenario field to the documentation? Maybe as part of the Maven, Bazel, Buck examples?

@blindpirate
Copy link
Contributor Author

Thanks @wolfs ! sure I will.

@blindpirate blindpirate merged commit 288d3cb into master Sep 18, 2020
@blindpirate blindpirate deleted the blindpirate/tool-home branch September 18, 2020 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow passing the MAVEN_HOME location or the location of mvn to MavenScenarioDefinition
2 participants