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

Maven Logic Improvements #195

Merged
merged 3 commits into from
Oct 11, 2022
Merged

Maven Logic Improvements #195

merged 3 commits into from
Oct 11, 2022

Conversation

dmikusa
Copy link
Contributor

@dmikusa dmikusa commented Oct 11, 2022

Summary

This fixes a couple bugs:

  1. A logic bug with selection of the maven command to run
  2. A logic bug during detection, which allows non-Java apps to be incorrectly detected

Resolves #194

Use Cases

Fix bugs.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

If Maven was not in the buildplan, then the command would never be set. This means the case where Maven is provided externally would never work.

The adjusted logic will unconditionally run the NoopMavenManger when maven is not in the buildplan. This will try to find Maven on the PATH. If it cannot find it on the path, then you get a reasonable error.

Signed-off-by: Daniel Mikusa <[email protected]>
There was a bug with the detect logic where the Maven buildpack was providing 'jvm-application-package' without any requirements. This paired with executable-jar and the application server buildpacks which had a requirement of 'jvm-application-package' and created a successful buildplan when there shouldn't have been one. This was originally added to support the case where someone might want to perform a build but not have the buildpack install Maven.

This PR changes the detection logic so that the 'jvm-application' is only added if a pom.xml file exists. This limits the functionality a bit, as you can only do a build-only build if you have a pom.xml but that seems reasonable at this time.

If necessary, we could circle back and adjust the logic further, but the plan is to hold off until there is a more defined used case that would require this functionality.

Additionally, this PR converts ioutil usage which has been deprecated.

Signed-off-by: Daniel Mikusa <[email protected]>
@dmikusa dmikusa added type:bug A general bug semver:patch A change requiring a patch version bump labels Oct 11, 2022
@dmikusa dmikusa requested a review from a team October 11, 2022 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:patch A change requiring a patch version bump type:bug A general bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect incorrectly detects non-maven apps
2 participants