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

feat: support for maven wrapper (mvnw) #50

Merged
merged 1 commit into from
Feb 10, 2020

Conversation

sfat
Copy link
Contributor

@sfat sfat commented Jan 24, 2020

  • Ready for review
  • Follows CONTRIBUTING rules
  • Reviewed by Snyk internal team

What does this PR do?

implementation for https://github.com/snyk/snyk/issues/957

@sfat sfat requested a review from a team as a code owner January 24, 2020 16:08
lib/index.ts Show resolved Hide resolved
lib/index.ts Outdated Show resolved Hide resolved
lib/index.ts Show resolved Hide resolved
@sfat sfat force-pushed the feat/support-mvn-wrapper branch 4 times, most recently from f24662d to 38a4b9a Compare January 27, 2020 17:16
@sfat sfat requested a review from orsagie January 27, 2020 17:26
lib/index.ts Show resolved Hide resolved
Copy link
Contributor

@orsagie orsagie left a comment

Choose a reason for hiding this comment

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

Looks great!

@sfat sfat force-pushed the feat/support-mvn-wrapper branch from 38a4b9a to 699df0e Compare January 28, 2020 18:25
// When we have `mvn`, we can run the subProcess from anywhere.
// However due to https://github.com/takari/maven-wrapper/issues/133, `mvnw` can only be run
// within the directory where `mvnw` exists
function calculateTargetFilePath(mavenCommand, root: string, targetPath) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@orsagie, @lili2311
fyi, this is will be a limitation because of that ticket I mentioned.
Do you think I should also add a line in the README.md?

Copy link
Contributor

Choose a reason for hiding this comment

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

isn't this what cwd is for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is, but the problem occurs when you specify a relative path to the targetFile will encounter a error as the mvnw is found, but the pom.xml is not present in that directory.
So, until that is fixed, you will not be able to specify a relative path for the targeFile (../somewhere-else) when mvnw is present.

That's what I meant by limitation.

Because at first I just went ahead and cwd with where the targetFile path is, but then I bumped into issues where the path to the pom.xml would be messed up while subProcess would be ran in both mvn and mvnw cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case let's add a check to see that mvnw is present in the root and propagate an error with instructions on how to properly run if that is not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will that work if you go with the targetFile approach being outside of that directory where you want to test your project?

@sfat sfat requested a review from orsagie January 28, 2020 18:45
@orsagie orsagie merged commit 62d511c into snyk:master Feb 10, 2020
@snyksec
Copy link

snyksec commented Feb 10, 2020

🎉 This PR is included in version 2.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants