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

Add a PinnedMavenInstallAnalyzer #4773

Merged
merged 2 commits into from Sep 2, 2022
Merged

Conversation

dhalperi
Copy link
Contributor

Fixes Issue

#4772

Description of Change

install.json is a new type of Maven lockfile commonly used
in Bazel Java projects. Implement virtual dependency scanning
for such files, modeled after the existing PipAnalyzer.

Have test cases been added to cover the new functionality?

Yes, I think so.

However, I was not able to run mvn -s settings.xml verify to success locally, so I did not yet add an IT. I also anticipate some changes to these tests in review.

In addition to the testing added in this PR, it worked on our
install.json file:
https://github.com/batfish/batfish/blob/6688b5b49ea695e7b566a0b70403396f580b2805/maven_install.json

@boring-cyborg boring-cyborg bot added ant changes to ant cli changes to the cli core changes to core maven changes to the maven plugin tests test cases utils changes to utils labels Aug 24, 2022
Copy link
Contributor Author

@dhalperi dhalperi left a comment

Choose a reason for hiding this comment

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

FYI, I modeled this after PipAnalyzer, with a lot of copy and paste.

I did follow my own standards for implementation details and debug logging. E.g., I deleted overridden Javadoc identical to parent.

I anticipate and am happy to iterate on the code to bring it up to local standards.

@dhalperi dhalperi force-pushed the maven-install branch 3 times, most recently from 6c2d5de to 9a00290 Compare August 25, 2022 22:28
`install.json` is a new type of Maven lockfile commonly used
in Bazel Java projects. Implement virtual dependency scanning
for such files, modeled after the existing PipAnalyzer.

In addition to the testing added in this PR, it worked on our
install.json file:
https://github.com/batfish/batfish/blob/6688b5b49ea695e7b566a0b70403396f580b2805/maven_install.json
@aikebah
Copy link
Collaborator

aikebah commented Aug 26, 2022

FYI, I modeled this after PipAnalyzer, with a lot of copy and paste.

I did follow my own standards for implementation details and debug logging. E.g., I deleted overridden Javadoc identical to parent.

I anticipate and am happy to iterate on the code to bring it up to local standards.

I'll leave the codestyle review to the various automated code reviews... results should appear soon now that I triggered the PR workflow. Hope to take a second look at the coding over the weekend.

@jeremylong any not automatically checked coding standards you'd like to enforce on the project?

@aikebah
Copy link
Collaborator

aikebah commented Aug 27, 2022

@dhalperi Up to now no issue, I've only glanced over and picked out some items, but going forward: please do not force-push, as that breaks the review cycle. With regular pushes only the changed bits need (re)review, with a force-push the entire change needs to be (re)reviewed as the delta to the already reviewed code is unclear.

…venInstallAnalyzer.java

Co-authored-by: Jeremy Long <jeremy.long@gmail.com>
@dhalperi
Copy link
Contributor Author

dhalperi commented Sep 1, 2022

Thanks for all the great feedback and useful improvements!

What else can I do?

@jeremylong
Copy link
Owner

@dhalperi thanks for the PR!

@jeremylong jeremylong merged commit 18ff526 into jeremylong:main Sep 2, 2022
@jeremylong jeremylong added this to the 7.1.3 milestone Sep 2, 2022
@aikebah aikebah linked an issue Sep 11, 2022 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ant changes to ant cli changes to the cli core changes to core maven changes to the maven plugin tests test cases utils changes to utils
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Bazel pinned maven_install.json files
3 participants