Skip to content

Don't overwrite blank (but non-empty) dominant values during mergeXpp3Dom #213

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 1 commit into from
Oct 10, 2022
Merged

Don't overwrite blank (but non-empty) dominant values during mergeXpp3Dom #213

merged 1 commit into from
Oct 10, 2022

Conversation

kwin
Copy link
Contributor

@kwin kwin commented Sep 11, 2022 •
edited
Loading

Compare with #212

@kwin kwin mentioned this pull request Sep 11, 2022
Closed
@kwin kwin changed the title Add test exposing that dominant blank (but non-empty) values are Don't overwrite blank (but non-empty) dominant values during mergeXpp3Dom Sep 11, 2022
@kwin kwin marked this pull request as ready for review September 11, 2022 10:55
@michael-o
Copy link
Member

I wonder what will fail with current downstream consumer tests

@kwin
Copy link
Contributor Author

kwin commented Sep 11, 2022

I wonder what will fail with current downstream consumer tests

Any particular tests you have in mind? I will try to run Maven ITs against this change.

@michael-o
Copy link
Member

I wonder what will fail with current downstream consumer tests

Any particular tests you have in mind? I will try to run Maven ITs against this change.

Run this patched version against Core plugins ITs.

@michael-o
Copy link
Member

I'd like to run tests on plugins. Does it suffice to update Maven Core with this patch?

@kwin
Copy link
Contributor Author

kwin commented Sep 11, 2022 •
edited
Loading

I modified Maven master to define
<plexusUtilsVersion>3.4.3-SNAPSHOT</plexusUtilsVersion> in the reactor pom.xml, build it and ran the Maven Core integration tests successfully against that distro.

@michael-o
Copy link
Member

I modified Maven master to define <plexusUtilsVersion>3.4.3-SNAPSHOT</plexusUtilsVersion> in the reactor pom.xml, build it and ran the Maven Core integration tests successfully against that distro.

Good, I will repeat with several core plugin ITs with 3.9.0-SNAPSHOT tomorrow.

Will this PR completely cover the downstream Maven issue?

@kwin
Copy link
Contributor Author

kwin commented Sep 11, 2022

Will this PR completely cover the downstream Maven issue?

No, it just allows to apply a workaround by giving a blank value in the plugin configuration to disable the default handling (FTR: the downstream issue is https://issues.apache.org/jira/browse/MNG-6434).

@michael-o michael-o force-pushed the bugfix/dominant-blank-values-overwritten-during-merge branch from ce417d5 to 748933c Compare October 10, 2022 13:18
@michael-o michael-o self-requested a review October 10, 2022 13:19
@sonatype-lift
Copy link

sonatype-lift bot commented Oct 10, 2022

⚠️ 22 God Classes were detected by Lift in this project. Visit the Lift web console for more details.

@michael-o michael-o merged commit 748933c into codehaus-plexus:master Oct 10, 2022
@kwin kwin mentioned this pull request Oct 10, 2022
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@michael-o michael-o Awaiting requested review from michael-o

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kwin @michael-o