Issue348 Log message only if trying to change maven property from ant… - #349
Issue348 Log message only if trying to change maven property from ant…#349jbindel wants to merge 1 commit into
Conversation
… task It is expected that some values may be different, specifically the special "ant.file" property can change between invocations of the antrun plugin (e.g., changing name from pom.xml to .flattened.pom.xml in some projects), and we do not want to issue warnings for such expected situations. Instead, we log an INFO message when we are skipping a value, and we log nothing for property values that are the same in Ant and Maven. The old behavior was to log a debug message, and new behavior is logging an info message only for rare things that may be surprising, but are generally innocuous and expected.
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
needs tests, probably an integration test but maybe you can do this with a unit test
| String mavenValue = mavenProperties.getProperty(key); | ||
| if (mavenValue != null) { | ||
| if (!mavenValue.equals(entry.getValue())) { | ||
| getLog().info("Ant property '" + key + "=" + entry.getValue() |
There was a problem hiding this comment.
I think a warning is appropriate here, at least some of the time. Possibly this can be a little pickier about when it warns but if it's warn or info, it should be warn.
| getLog().warn("Ant property '" + key + "=" + mavenProperties.getProperty(key) | ||
| + "' clashes with an existing Maven property, SKIPPING this Ant property propagation."); | ||
| String mavenValue = mavenProperties.getProperty(key); | ||
| if (mavenValue != null) { |
There was a problem hiding this comment.
This part, checking if the maven property value matches the ant property value, seems like a good idea.
There was a problem hiding this comment.
Pull request overview
This PR adjusts AntRunMojo.copyProperties(Project, MavenProject) logging to reduce build output noise when exportAntProperties encounters Maven properties that already exist, aligning behavior with Issue #348 (avoid per-property WARN spam, only surface surprising mismatches).
Changes:
- Stores the existing Maven property value in a local variable before deciding whether to propagate an Ant property.
- Skips propagation when a Maven property already exists, and logs at INFO only when the Ant and Maven values differ.
- Avoids logging anything when Ant and Maven values are the same.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| String mavenValue = mavenProperties.getProperty(key); | ||
| if (mavenValue != null) { | ||
| if (!mavenValue.equals(entry.getValue())) { | ||
| getLog().info("Ant property '" + key + "=" + entry.getValue() | ||
| + "' clashes with an existing Maven property value '" + mavenValue | ||
| + "', SKIPPING this Ant property propagation."); | ||
| } | ||
| continue; | ||
| } |
| if (!mavenValue.equals(entry.getValue())) { | ||
| getLog().info("Ant property '" + key + "=" + entry.getValue() | ||
| + "' clashes with an existing Maven property value '" + mavenValue | ||
| + "', SKIPPING this Ant property propagation."); | ||
| } |
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
I think the original code only prints the key clashes. The new code prints values as well which can leak sensitive information into log files. Only the key should be logged.
|
I appreciate the suggestions, and I'll plan to address them soon. |
fixes #348
It is expected that some values may be different, specifically the special "ant.file" property can change between invocations of the antrun plugin (e.g., changing name from pom.xml to .flattened.pom.xml in some projects), and we do not want to issue warnings for such expected situations. Instead, we log an INFO message when we are skipping a value, and we log nothing for property values that are the same in Ant and Maven.
The old behavior was to log a debug message, and new behavior is logging an info message only for rare things that may be surprising, but are generally innocuous and expected.
Following this checklist to help us incorporate your
contribution quickly and easily:
Note that commits might be squashed by a maintainer on merge.
This may not always be possible but is a best-practice.
mvn verifyto make sure basic checks pass.A more thorough check will be performed on your pull request automatically.
mvn -Prun-its verify).If your pull request is about ~20 lines of code you don't need to sign an
Individual Contributor License Agreement if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.