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

Fixes #145, #148 add codes to parse the <version> for the dependency in profile which is managed by <dependencyManagement> #155

Merged
merged 13 commits into from
Jun 5, 2020

Conversation

yangnuoyu
Copy link
Contributor

@yangnuoyu yangnuoyu commented May 27, 2020

@yangnuoyu yangnuoyu marked this pull request as ready for review May 27, 2020 21:33
@stephaniewang526
Copy link
Contributor

stephaniewang526 commented Jun 1, 2020

Could you add a test case that maps to our Java client library scenario?

Here's an example: https://github.com/googleapis/java-pubsub/blob/master/google-cloud-pubsub/pom.xml#L174 (ignore the added version in profile)

  1. Configuration needs to be at minimum
    <flattendependencymode>all<flattendependencymode> and <flattenmode>oss</flattenmode> (complete config)

  2. Dependency version managed through BOM in the parent pom

…oss to profile-with-deps-inherit-parent-depMgmt-flatten-dep-all-oss-bom
@stephaniewang526
Copy link
Contributor

LGTM

@stephaniewang526
Copy link
Contributor

@saturnism @olamy PTAL

@saturnism
Copy link
Contributor

+1 to moving the logic away from effective pom. but it still misses the question of what if the dep management section is kept.

strippedDependencies.add( flattenedDep );
}
}
if (!strippedDependencies.isEmpty() || !isEmpty(profile.getRepositories())) {
Copy link
Member

Choose a reason for hiding this comment

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

style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

@olamy olamy left a comment

Choose a reason for hiding this comment

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

except some nit picking changes look good even including good it test

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

I'm a little uncertain about what code styles and license rules apply here. @stephaniewang526 do you know?

@yangnuoyu yangnuoyu requested a review from olamy June 5, 2020 06:14
@olamy
Copy link
Member

olamy commented Jun 5, 2020

@elharo historically the maven codestyle is used. But it's more relax here as well so the build do not fail for that.

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.

Unable to fetch dependency version in <profile> when the dependency is managed by <dependencyManagement>
5 participants