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

Fix for issue #582 - update-properties does not work across parent-child pom #616

Merged
merged 1 commit into from Jul 25, 2022

Conversation

prodj17
Copy link

@prodj17 prodj17 commented Jul 12, 2022

Fixed #582
o Updated PomHelper to validate version properties also across parents.

@prodj17
Copy link
Author

prodj17 commented Jul 13, 2022

@slawekjaranowski do you think you can find some time to look into this?

Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

  • it looks like we can add a unit tests for PomHelper, it should be easier to check coverage by test
  • what will be if in parent we will have dependencyManagment and in child dependency without version, and property
  • what will be if in parent we will have dependencyManagment and in child no dependency, but only property

disclosure - I'm not know this code ... and logic looks quite complex, so will be good if someone else looks at it

src/it-repo/dummy-parent-issue-582-1.0.pom Outdated Show resolved Hide resolved
src/it/it-update-properties-issue-582/pom.xml Outdated Show resolved Hide resolved
src/it/it-update-properties-issue-582/verify.bsh Outdated Show resolved Hide resolved
@prodj17
Copy link
Author

prodj17 commented Jul 13, 2022

disclosure - I'm not know this code ... and logic looks quite complex, so will be good if someone else looks at it

The change in logic is actually quite simple, yet for some reason github diff messed up. If you hide whitespaces changes the diff looks a lot better - the changes are highlighted correctly.

The logic of dependencyManagement vs property stays the same as for a single pom. The only change there is to actually iterate over the parent poms and process those as well when looking for properties.

@olamy @bmarwell I've seen you made changes to PomHelper more recently. Do you guys think you can look into it?

@slawekjaranowski
Copy link
Member

please change commit message to something more related what we do, eg like:

update-properties does not work across parent-child pom

pleas also squash to one commit

@prodj17
Copy link
Author

prodj17 commented Jul 14, 2022

please change commit message to something more related what we do, eg like:

update-properties does not work across parent-child pom

pleas also squash to one commit

@slawekjaranowski done

@slawekjaranowski slawekjaranowski changed the title Fix for issue #582 Fix for issue #582 - update-properties does not work across parent-child pom Jul 14, 2022
Copy link
Member

@slawekjaranowski slawekjaranowski left a comment

Choose a reason for hiding this comment

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

ok, looks ok for me.

I will wait a few days for other comments.

@prodj17
Copy link
Author

prodj17 commented Jul 18, 2022

ok, looks ok for me.

I will wait a few days for other comments.

@slawekjaranowski Can this be merged now since there are no more comments? Is there a timeframe for the next release of the plugin? Anything that I need to do for the release?

@prodj17
Copy link
Author

prodj17 commented Jul 25, 2022

@slawekjaranowski It has been more than a week and there are no comments. Could we merge this PR?

@slawekjaranowski
Copy link
Member

Ok.
last request, can you squash to single commit.
Of course I can do it for you.

Fixes mojohaus#582
 o Updated PomHelper to validate version properties also across parents.
@prodj17
Copy link
Author

prodj17 commented Jul 25, 2022

Ok. last request, can you squash to single commit. Of course I can do it for you.

@slawekjaranowski
Done. Can this be merged now?

@slawekjaranowski slawekjaranowski merged commit 20019d5 into mojohaus:master Jul 25, 2022
@slawekjaranowski
Copy link
Member

@prodj17 - thanks

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.

update-properties does not work across parent-child pom
2 participants