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

Behavior change in incrementMinorIfNotOnRelease incrementer when on release branch #391

Closed
mateuszkwiecinski opened this issue Mar 29, 2021 · 14 comments

Comments

@mateuszkwiecinski
Copy link

mateuszkwiecinski commented Mar 29, 2021

Context:
I'm using incrementMinorIfNotOnRelease incrementer by setting

scmVersion.versionIncrementer = PredefinedVersionIncrementer.versionIncrementerFor("incrementMinorIfNotOnRelease")

Repro steps:

  1. Checkout a commit tagged release-1.2.0 on a release/1.2.0 branch.
  2. Run ./gradlew currentVersion tasks and ensure it prints 1.2.0
  3. Make an additional commit
  4. Run ./gradlew currentVersion task again

Expected result:
Task completes with 1.2.1-SNAPSHOT output, as it was in 1.12.1

Actual result:
Task prints 1.3.0-SNAPSHOT

It looks like an oversight as a tag related function is used to set branch related property
de4ee1e#diff-50b4d296a39fb72c511d8a25cd41aa5ccb5bc33637d078f6fbb61bf9158108d3L24

It seem like the workaround/fix is to set releaseBranchPattern explicitly, however I still haven't managed to make it work 👀

In case when needed: test failing on 1.13.1 + plugin config

@mateuszkwiecinski mateuszkwiecinski changed the title Behavior change in incrementMinorIfNotOnRelease Behavior change in incrementMinorIfNotOnRelease incrementer when on release branch Mar 29, 2021
@bgalek
Copy link
Member

bgalek commented Mar 29, 2021

I'll look at this tomorrow! Thanks for detailed issue submission!

@bgalek
Copy link
Member

bgalek commented Mar 29, 2021

I've made a blind guess https:/allegro/axion-release-plugin/pull/393/files (didn't test it yet) what do You think @mateuszkwiecinski ?

@bgalek
Copy link
Member

bgalek commented Mar 29, 2021

@mateuszkwiecinski a quick "manual" test seems not ok, gonna check it out tomorrow! :)

@bgalek
Copy link
Member

bgalek commented Mar 31, 2021

@mateuszkwiecinski I've tested again, I think that this PR fixes the issue ;)

@bgalek
Copy link
Member

bgalek commented Mar 31, 2021

@mateuszkwiecinski could you test it from this branch? I'll need some time to create a useful tests for this feature but also - I would like to fix broken release ;)

@mateuszkwiecinski
Copy link
Author

mateuszkwiecinski commented Mar 31, 2021

Sure, I should be able to test it later today (somewhen around late evening), if not I'll do it as the very first thing tomorrow morning 👀

Is there a snapshot version available or should I build and publish the plugin locally?

Also, could you explain why it is expected to use tag config at release branch naming? Previously release branch format and tag name format were considered two separate things. AFAIU these are now unified. Is it expected to have branches named in a v/1.2.3 way?

(or at least that's what I read from: config.releaseBranchPattern = context.isLegacyDefTagnameRepo() ? TagPrefixConf.DEFAULT_LEGACY_PREFIX + '/.+' : TagPrefixConf.prefix() + '/.+')

According to what I understand from the code it will work in my case and it'll recognise release/1.2.3 branch, but it'll be broken for new users, and probably will work in an unexpected way if consumers will update tag prefix, no?

@bgalek
Copy link
Member

bgalek commented Mar 31, 2021

There is no way (AFAIK) to have gradle plugins snapshots. So please use mavenLocal().

@bgalek
Copy link
Member

bgalek commented Mar 31, 2021

https://axion-release-plugin.readthedocs.io/en/latest/configuration/version/#incrementminorifnotonrelease
Yes, the new default assumes v/1.2.3 branch naming pattern. I'll update release notes.

Before my fix incrementMinorIfNotOnRelease releaseBranchPattern argument used only new default value (v/1.2.3). I've passed a flag to indicate is it old default (release-) or new (v) and then decide. Of course, when somebody uses a custom releaseBranchPattern this should still work (incrementer would receive overridden version).

@mateuszkwiecinski
Copy link
Author

There is no way (AFAIK) to have gradle plugins snapshots

They can't be published on Gradle Plugin Portal indeed, but they can be published to any other maven repository. Most projects use Sonatype snapshot repository

Yes, the new default assumes v/1.2.3 branch naming pattern

Thanks for confirming!

Of course, when somebody uses a custom releaseBranchPattern this should still work

Ok, I briefly tried that initially when trying to work around the issue, but it didn't work for me 🤔
I assumed some context object isn't passed correctly, but I abandoned the idea and reported this issue instead. I'll make sure I can still override releaseBranchPattern with custom values

@bgalek
Copy link
Member

bgalek commented Mar 31, 2021

Thanks for the idea with sonatype - it seems kinda obvious now as You mentioned it!
I use local maven for testing - don't see why I couldn't use external one 😅

Please do check if everything seems all right on your end!

@mateuszkwiecinski
Copy link
Author

mateuszkwiecinski commented Mar 31, 2021

@bgalek So: I managed to run tests linked in the original post, they all passed which means the legacy format is properly recognized now 👍

As mentioned above, I had tried to set custom releaseBranchPattern but I couldn't make it work.
Here's sample repro project: https:/mateuszkwiecinski/axion_playground

it has following repository state:
image

and as you can see in the repository, my configuration looks as follows
(Groovy)

scmVersion {
    tag {
        prefix = 'ohoh'
    }
    
    versionIncrementer = PredefinedVersionIncrementer.versionIncrementerFor('incrementMinorIfNotOnRelease')
    releaseBranchPattern = Pattern.compile('^random(/.*)?$')
}

I expected that the IfNotOnRelease part somehow relates to releaseBranchPattern, but clearly it doesn't and I should have used something like:
(Kotlin)

versionIncrementer = PredefinedVersionIncrementer.versionIncrementerFor("incrementMinorIfNotOnRelease", mapOf("releaseBranchPattern" to "^random(/.*)?$".toRegex().toPattern())

Is that expected? What's the purpose of VersionConfig#releaseBranchPattern? I couldn't find any usages of it 🤔

Other than that, the changes on your branch seems to be working fine in all of my usecases (except #392)

@bgalek
Copy link
Member

bgalek commented Mar 31, 2021

Than You for testing ang analysis!

I'm gonna dig into VersionConfig#releaseBranchPattern, docs clearly state:

If incrementMinorIfNotOnRelease rule is activated then the property ``releaseBranchPattern`` is used to match the release
branch. By default it's set to 'v/.+'.

but it's from 2015 and maybe something broke. I'm wondering if before 1.13.x this was working?

bgalek added a commit that referenced this issue Mar 31, 2021
legacy tag prefiex preserved in incrementMinorIfNotOnRelease #391
@bgalek
Copy link
Member

bgalek commented Mar 31, 2021

I've merged the fix as it preserves old functionality. I'll get back to this issue when confirmed that the bug was introduced in 1.13.x.

@mateuszkwiecinski
Copy link
Author

mateuszkwiecinski commented Mar 31, 2021

I'm wondering if before 1.13.x this was working?

It did not 😛 I tested on 1.11.0 and 1.12.1 and the VersionConfig#releaseBranchPattern option doesn't change the release branch pattern anywhere.

I only started using the option in 1.13.1 when I tried to work around the change introduced 1.13.x with default naming convention (or actually to overcome this precise issue).

I created separate issue: #396 and I'm gona close this one as you fixed the original problem already. Thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants