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

Resolves #474: Add property support to UseDepVersion #878

Conversation

jarmoniuk
Copy link
Contributor

  • adding a new parameter, processProperties, which will also update property values if dependencies are controlled by properties
  • the above parameter will be turned off in order to retain backwards compatibility

The behaviour of the property is described here:

/**
 * <p>Will augment normal processing by, if a dependency value is set using a property, trying to update
 * the value of the property.</p>
 * <p>If the property value is specified directly, will process it normally (as with {@code processProperties} being
 * {@code false}. If the property being updated is redefined in the reactor tree, will only change the property
 * value which lies closest to the dependency being updated. If the same property is also used to set
 * the value of another dependency, will not update that property value, and log a warning instead.
 * Finally, if the property value is specified in a parent file which is outside of the project, will log
 * a warning message.</p>
 * <p>Default is {@code false}.</p>
 *
 * @since 2.14.2
 */

also, the behaviour is also "described" in the unit tests.

@slawekjaranowski please review

By the way, I've put 2.14.2 tentatively, but because of the nature of this PR, I'd understand if it did not fit into the release.

@jarmoniuk jarmoniuk changed the title Resolves #474: Add property support for UseDepVersion Resolves #474: Add property support to UseDepVersion Dec 23, 2022
@jarmoniuk jarmoniuk force-pushed the issue-474-use-dep-version-parameters branch from 3828fd8 to 76fffa6 Compare December 23, 2022 12:47
@jarmoniuk
Copy link
Contributor Author

Manual testing...

@jarmoniuk jarmoniuk marked this pull request as draft December 23, 2022 12:55
@jarmoniuk jarmoniuk marked this pull request as ready for review December 23, 2022 13:27
@jarmoniuk jarmoniuk force-pushed the issue-474-use-dep-version-parameters branch from 76fffa6 to 4ff2b58 Compare December 23, 2022 13:28
@jarmoniuk
Copy link
Contributor Author

Changed it so that it doesn't scan the whole tree if we're not processing properties.

Unfortunately, there's one problem I still don't know how to tackle -- maven unnecessarily descends into children even if the plugin could have made all necessary changes starting from root level.

Do you know how to change that so that Maven doesn't run recursively -- from the plugin?

@slawekjaranowski
Copy link
Member

I would like to postpone this for next minor release ... in next year 😄

@jarmoniuk
Copy link
Contributor Author

That's OK 😄

@jarmoniuk jarmoniuk force-pushed the issue-474-use-dep-version-parameters branch from 4ff2b58 to ddd5888 Compare December 23, 2022 14:33
@jarmoniuk
Copy link
Contributor Author

So I guess that's a nice PR for 🎄😄

@jarmoniuk jarmoniuk force-pushed the issue-474-use-dep-version-parameters branch from ddd5888 to d803ed7 Compare December 23, 2022 14:35
@jarmoniuk
Copy link
Contributor Author

Note to self: https://maven.apache.org/developers/mojo-api-specification.html#The_Descriptor_and_Annotations

Since there's time, I'll polish it a bit.

@jarmoniuk jarmoniuk force-pushed the issue-474-use-dep-version-parameters branch 2 times, most recently from 40b4e4c to c2840c4 Compare December 24, 2022 08:04
@jarmoniuk
Copy link
Contributor Author

Changed to aggregate mode; this change spurred the creation of a new enhancement: #880

@jarmoniuk jarmoniuk marked this pull request as draft December 25, 2022 12:34
@jarmoniuk
Copy link
Contributor Author

jarmoniuk commented Dec 25, 2022

Temporarily converting to draft: let's see if I we can use maven (to do the recursion) and plugin context instead.

Nope, it would make things much simpler, but -- unfortunately, the modules seem to be built in the wrong order - from top to down.

@jarmoniuk jarmoniuk marked this pull request as ready for review December 25, 2022 13:30
- adding a new parameter, processProperties, which will also update property values if dependencies are controlled by properties
- changed to aggregator mode
@jarmoniuk jarmoniuk force-pushed the issue-474-use-dep-version-parameters branch from c2840c4 to 4d81dc7 Compare January 2, 2023 21:32
@slawekjaranowski slawekjaranowski merged commit 901aac0 into mojohaus:master Jan 3, 2023
@slawekjaranowski slawekjaranowski added this to the next-release milestone Jan 3, 2023
@jarmoniuk jarmoniuk deleted the issue-474-use-dep-version-parameters branch January 3, 2023 20:07
srowen pushed a commit to apache/spark that referenced this pull request Mar 3, 2023
### What changes were proposed in this pull request?
This pr aims upgrade `versions-maven-plugin` to 2.15.0

### Why are the changes needed?
New version bring some improvements like:
- mojohaus/versions#898
- mojohaus/versions#883
- mojohaus/versions#878
- mojohaus/versions#893

and some bug fix:
- mojohaus/versions#901
- mojohaus/versions#897
- mojohaus/versions#891

The full release notes as follows:
- https://github.com/mojohaus/versions/releases/tag/2.15.0

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?

- GA `Dependencies test` should work normally
- Manually check `./dev/test-dependencies.sh --replace-manifest`, run successful

Closes #40248 from LuciferYang/SPARK-42648.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Sean Owen <srowen@gmail.com>
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.

Add support for property defined version update in the goal use-dep-version
2 participants