[MRELEASE-1109] Snapshot detection and support for versions like `${revision}${sha1}${changelist}` #202

mkolesnikov · 2023-11-29T20:46:48Z

Following this checklist to help us incorporate your
contribution quickly and easily:

Make sure there is a JIRA issue filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
Each commit in the pull request should have a meaningful subject line and body.
Format the pull request title like [MRELEASE-XXX] - Fixes bug in ApproximateQuantiles,
where you replace MRELEASE-XXX with the appropriate JIRA issue. Best practice
is to use the JIRA issue title in the pull request title and in the first line of the
commit message.
Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
Run mvn clean verify -Prun-its to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.

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.

I hereby declare this contribution to be licenced under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.

…snapshot_detection # Conflicts: # maven-release-manager/src/test/resources/projects/rewrite-for-release/pom-with-parent-and-cifriendly-expressions/expected-pom.xml

...-manager/src/main/java/org/apache/maven/shared/release/phase/RewritePomsForReleasePhase.java

…ing comments.

...se-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java

...release-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomModel.java

…h `replace`. Added more tests after using this version for a while in a real project.

# Conflicts: # maven-release-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java # maven-release-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomModel.java # maven-release-manager/src/test/resources/projects/rewrite-for-release/pom-with-parent-and-cifriendly-expressions/expected-pom.xml

maven-release-manager/src/main/java/org/apache/maven/shared/release/util/CiFriendlyVersion.java

michael-o · 2024-06-04T08:42:49Z

maven-release-manager/src/main/java/org/apache/maven/shared/release/util/CiFriendlyVersion.java

+        // try to rewrite property if CI friendly expression is used
+        String ciFriendlyPropertyName = extractPropertyFromExpression(versionElement);
+        if (properties != null) {
+            String sha1 = properties.getProperty(SHA_1, System.getProperty(SHA_1, ""));


Was System.getProperty() used before? We tend not to rely on system properties, but only on Maven properties.

@cstamas @slawekjaranowski

The sha1 property for CI is usually passed from a command-line. This class is an extract of everything related to CI Friendly. See, the usage of this method, it's used in JDomModel which does not have Maven project reference.
The goal of CiFriendlyVersion is a small clean up without a big refactoring.

This is a contradiction to https://github.com/apache/maven/blob/5e977034565ffc50c6b073f21fab50fae71705dd/maven-api-impl/src/main/java/org/apache/maven/internal/impl/model/DefaultModelVersionProcessor.java#L51-L62 because they can only come from model or Maven user properties...

I'll revert it back to System those properties do not exist in MavenProject because the current maven version for this plugin defined as <mavenVersion>3.2.5</mavenVersion> but you've put a link to the changes made 2 months ago.

That is not correct. The class has been modified long time ago by me. Check its history. I can find the JIRA, if you want. The release of Maven Release will upgrade to 3.6.3 anyway.

Here is the change: https://issues.apache.org/jira/browse/MNG-7658. Model properties do get overwritten with the user properties. It is expected to work properly.

But it does not with the current dependencies defined in this project :) Just run the integration test and you will see it maven-release-plugin/src/it/projects/prepare/ci-friendly-multi-module

Then the huge problem is the current dependencies defined in the pom.xml. It's just dependency hell that does not work if everything is set up as it's defined.
First of all, mavenVersion=3.2.5, the project even does not compile with maven-3.2.5 binary, it is immediately failed by maven-enforce-plugin and requires 3.6.3 version.
Then if you try to bump mavenVersion to 3.6.3 and the binary to the same version, it fails again.

Taking all this dependency incompatibility issues, please do not refer to the recent changes, they just don't work.

Let's pause this until Maven Release is updated to 3.6.3. I will work in this soon.

Okay, I will update the PR later, it will be nice if you notice me in this thread as soon as you finish maven version upgrade.

...se-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomProperties.java

michael-o · 2024-06-04T11:21:20Z

maven-release-manager/src/main/java/org/apache/maven/shared/release/util/CiFriendlyVersion.java

+
+    public static void rewriteVersionAndProperties(
+            String version, String versionElement, JDomProperties jDomProperties, Properties mavenProperties) {
+        // try to rewrite property if CI friendly expression is used


Are these project properties, user properties or a merge of both?

These are only the maven properties defined in the pom.xml

That should be enough because the processor I have shown you should merge this for you.

It does not work, they are not merged. I did debugging and saw it my own eyes.

michael-o · 2024-06-04T17:26:14Z

...release-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomModel.java

+                        version,
+                        versionElement.getTextNormalize(),
+                        (JDomProperties) getProperties(),
+                        mavenProject.getProperties());


@cstamas @slawekjaranowski @gnodet Do you guys know from the top of your heads whether this contains merged props (model + user) or model only?

The code reverted back to using System.getProperty as it's the only way to get sha1 value from mavenOpts, see the integration test.

… but nothing was provided by a user

Mikhail Kolesnikov and others added 4 commits November 29, 2023 19:47

MRELEASE-1109 full support of CI Friendly expression

21df8de

MRELEASE-1109 patch JDomModel

ab5c8d5

Merge branch 'bugfix/MRELEASE-1109_patch' into feature/MRELEASE-1109_…

466383d

…snapshot_detection # Conflicts: # maven-release-manager/src/test/resources/projects/rewrite-for-release/pom-with-parent-and-cifriendly-expressions/expected-pom.xml

MRELEASE-1109 Full support for revision sha1 and changelist properties.

0ad7bb6

mkolesnikov mentioned this pull request Nov 29, 2023

[MRELEASE-1109] Support CI friendly versions #198

Merged

kwin requested review from kwin and michael-o November 30, 2023 13:02

kwin reviewed Nov 30, 2023

View reviewed changes

...-manager/src/main/java/org/apache/maven/shared/release/phase/RewritePomsForReleasePhase.java Outdated Show resolved Hide resolved

mkolesnikov added 4 commits November 30, 2023 18:22

MRELEASE-1109 single revision property

ef93eec

MRELEASE-1109 reverted redundant changes

27791ef

MRELEASE-1109 fixed format

66f51ce

MRELEASE-1109 added asserts for pom.xml.tag files and removed mislead…

dbcbb08

…ing comments.

kwin approved these changes Dec 3, 2023

View reviewed changes

michael-o reviewed Dec 3, 2023

View reviewed changes

...se-manager/src/main/java/org/apache/maven/shared/release/phase/AbstractRewritePomsPhase.java Outdated Show resolved Hide resolved

michael-o reviewed Dec 3, 2023

View reviewed changes

...release-manager/src/main/java/org/apache/maven/shared/release/transform/jdom2/JDomModel.java Outdated Show resolved Hide resolved

mkolesnikov added 3 commits June 3, 2024 17:31

MRELEASE-1109 addressed review comments and replaced replaceAll wit…

4032941

…h `replace`. Added more tests after using this version for a while in a real project.

MRELEASE-1109 removed redundant comment

6b4fc81

michael-o self-requested a review June 3, 2024 18:23

michael-o requested changes Jun 4, 2024

View reviewed changes

mkolesnikov added 2 commits June 4, 2024 12:25

MRELEASE-1109 renamed SHA1 constant

fbc3ac2

MRELEASE-1109 JDomModel should know about current MavenProject

0be8e5a

michael-o reviewed Jun 4, 2024

View reviewed changes

mkolesnikov added 3 commits June 5, 2024 03:21

MRELEASE-1109 Using scmRevision as sha1 value if the property defined…

56fe3ed

… but nothing was provided by a user

MRELEASE-1109 fixed integration test

28ebea0

MRELEASE-1109 clean up and format

fe02232

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRELEASE-1109] Snapshot detection and support for versions like `${revision}${sha1}${changelist}` #202

[MRELEASE-1109] Snapshot detection and support for versions like `${revision}${sha1}${changelist}` #202

mkolesnikov commented Nov 29, 2023

michael-o Jun 4, 2024

mkolesnikov Jun 4, 2024

michael-o Jun 4, 2024

mkolesnikov Jun 4, 2024

mkolesnikov Jun 4, 2024

michael-o Jun 5, 2024

michael-o Jun 5, 2024

mkolesnikov Jun 5, 2024 •

edited

michael-o Jun 5, 2024

mkolesnikov Jun 5, 2024

michael-o Jun 4, 2024

mkolesnikov Jun 5, 2024

michael-o Jun 5, 2024

mkolesnikov Jun 5, 2024

michael-o Jun 4, 2024

mkolesnikov Jun 5, 2024 •

edited

[MRELEASE-1109] Snapshot detection and support for versions like ${revision}${sha1}${changelist} #202

Are you sure you want to change the base?

[MRELEASE-1109] Snapshot detection and support for versions like ${revision}${sha1}${changelist} #202

Conversation

mkolesnikov commented Nov 29, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkolesnikov Jun 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkolesnikov Jun 5, 2024 • edited

Choose a reason for hiding this comment

[MRELEASE-1109] Snapshot detection and support for versions like `${revision}${sha1}${changelist}` #202

[MRELEASE-1109] Snapshot detection and support for versions like `${revision}${sha1}${changelist}` #202

mkolesnikov Jun 5, 2024 •

edited

mkolesnikov Jun 5, 2024 •

edited