analyze_commits bumps several versions

jhbertra commented 4 years ago

It seems that analyze_commit bumps the patch / minor / major version for every commit it finds that should bump that version.

This seems like unexpected behaviour. The behaviour I was expecting is:

if commit_range.contains_breaking_change() then
    next_version = "#{current_major + 1}.0.0"
else if commit_range.contains_type("feat") then
    next_version = "#{current_major}.#{current_minor + 1}.0"
else if commit_range.contains_type("fix") then
    next_version = "#{current_major}.#{current_minor}.#{current_patch + 1}"
end

But it seems like the actual logic is:

next_major = current_major + commit_range.count_breaking_changes()
next_minor = current_minor + commit_range.count_type("feat")
next_patch = current_patch + commit_range.count_type("fix")
next_version = "#{next_major}.#{next_minor}.#{next_patch}"

Edit I found the source code responsible for this: https://github.com/xotahal/fastlane-plugin-semantic_release/blob/master/lib/fastlane/plugin/semantic_release/actions/analyze_commits.rb#L87

I think it would make sense to allow multiple changes to be bundled in the same atomic version increment - jumping the version from e.g. 3.6.3 to 3.6.5 just because the analyzed commit range contains two fix commits doesn't seem necessary.

hanno-jonlay commented 4 years ago

This was also a little surprising to me, and I think I'd support this suggestion.

xotahal commented 4 years ago

I was thinking about this as well.

But just about the BRAKING CHANGE actually. If the commit range contains "BREAKING_CHANGE" 7 times it just doesn't make sense to bump version from 1.4.34 -> 8.0.0. I agree with this.

But otherwise, I believe that it should bump every single fix/feat.

It probably depends on how you work with repository.

If you have two commits like this:

- fix: fix app crash when press submit button
- fix: fix visibility of next button if a credit card selected

I believe these two changes should bump the version from e.g. 3.6.3 to 3.6.5 because those are actually two changes that affect using of the app.

jhbertra commented 4 years ago

Hm, I'm not sure I buy that reasoning. The purpose of semantic versioning is to communicate the nature of changes and to set expectations for the burden of upgrading. The number of bugfixes in a patch release doesn't matter to the consumer of a library - all that is important is knowing how much effort and planning is required to do the upgrade. So I don't really see why encoding the number of features or bugfixes in the version bump would be desireable behavior. No reason to skip from 6.3.1 to 6.3.15 because you fixed 14 bugs in a release for example. Obviously not a problem if you do continuous deployment and every push to master triggers a release, but for scheduled releases this is a bit odd.

xotahal commented 4 years ago

As you said:

all that is important is knowing how much effort and planning is required to do the upgrade

When I see: A. 6.3.1 -> 6.3.2 - I know I can upgrade without any problem B. 6.3.1 -> 6.3.15 - I know I can upgrade without any problem

What are we actually talking about? 😀

hanno-jonlay commented 4 years ago

Hmm.

I understand that semver means that "we no longer need to worry about the version numbers".

But I think if I see a plugin on npmjs went from 6.3.1 to 6.3.15, I ask myself "where are versions 6.3.2 to 6.3.14?!"

When you are releasing daily/intermittent builds, the "rapidly jumping version numbers" can be a bit confusing.

And as you say, jumping from 1.4.34 -> 8.0.0 if there are a bunch of breaking changes is not how things should probably behave.

Possible property

So I was thinking of 3 possible options with versioning approach. The naming is not quite right, but just to give a general idea of how a versioning_mode property might work:

maximal = Current approach, can be left as the default if we prefer - bump the version for every single commit (6.3.1 -> 6.3.15)
minimal = As we're discussing here, even if there are 14 fix commits, the version will only jump from: 6.3.1 -> 6.3.2
locked = Allows the analyze_commits checks to pass, and will build a changelog for all the commits since the last tagged release, but leaves the version number unchanged.

FYI, the locked option is something we are using for our own build process at the moment while in an alpha release process for an iOS app - we don't want the version number to increase because if we do, TestFlight will require us to get Apple to approve the new version number. So we just version by bumping the release number.

In summary

I still think that minimal is the correct approach to the plugin, rather than the current maximal.
Given the choice, I would make minimal (and locked) the only ways to use the plugin (default to minimal).
But maybe if we introduce the extra options, it would allow people to customise to their needs.

What do you think? I'm happy to have a go at some of this, but a behavioural change like this would need your agreement, I guess 😄

jhbertra commented 4 years ago

@jonlay good call on the locked behavior - I actually ended up implementing that for alpha builds too (with an incrementing prerelease variant like alpha-1) very useful. That way breaking changes in alpha don't cause LTS to go from version 2 to version 4.

xotahal commented 4 years ago

Guys this is a current version of chrome - 78.0.3904.87

I don't believe they reached this number by increasing with number 1.

I am not saying it is good. I am not saying it is bad. What I try to say is there are as many opinions as there are people. I'd like to skip this conversation because I had plenty of these at every single company I was working for. They always had their own approach how to do versioning. And of course, everyone thinks they approach is the best.

I believe there is no objective reason against doing versioning like this:

6.3.1 -> 6.3.15
6.3.1 -> 6.3.2
6.3.1 -> 12.0.0

All these versions look as they follow the semver and conventional commits. Everything else is just our personal emotion.

Do you like 6.3.1 -> 6.3.15? No Do I like 6.3.1 -> 6.3.2? No Do we like 6.3.1 -> 12.0.0? No

Who does care about what we like? Nobody! 😀

Let's do this

I am open to add some extra options. So everyone can achieve what they think is best for them.

I would do the maximal/minimal approach but on the level of major, minor, patch. So I can do minimal for major and maximal for others. You guys can do minimal for all of them.

Everyone is happy 🎉

About the locked option: There is already RELEASE_LAST_VERSION variable which you can use in your Fastfile. That gives you what you need, correct? If it is not enough we can add more granularity similar to RELEASE_NEXT_VERSION (RELEASE_LAST_MAJOR_VERSION, RELEASE_LAST_MINOR_VERSION, etc)

hanno-jonlay commented 4 years ago

Totally fine by me. I agree that providing an option is better than setting an opinionated default.

Bumping mode

I would do the maximal/minimal approach but on the level of major, minor, patch. So I can do minimal for major and maximal for others. You guys can do minimal for all of them.

Sounds good. I won't have a ton of time to write this in the next week or two, but happy to take a look if you do a draft, or to try a version myself, if you want to explain in a bit more detail.

Locked versions

For context, I'm indeed using RELEASE_LAST_VERSION, but I find that the approach to making use of that can be quite hacky, overwriting the RELEASE_NEXT_VERSION with RELEASE_LAST_VERSION:

if version_lock == true
    # Approval from Apple is required for all builds with a new version number
    # So if we are releasing Alpha builds, we want to keep the *version* number unchanged, and simply bump the *release* number
    # This condition won't be used for beta and other builds, on which we will allow conventional_commits to manage our version numbering automatically
    # Ideally, the conventional commits plugin would support this by default
    var_next_version_number = lane_context[SharedValues::RELEASE_LAST_VERSION]
    lane_context[SharedValues::RELEASE_NEXT_VERSION] = lane_context[SharedValues::RELEASE_LAST_VERSION] # Second override this so that the title of release notes generated by the plugin is correct
  else
    var_next_version_number = lane_context[SharedValues::RELEASE_NEXT_VERSION]
  end

That's why I suggested the possibility of including a "locked version" option so that this can be elegantly handled by passing a single versioning_mode: 'locked' property for any lane in the Fastfile which needs to lock the version numbers.

So, not a dealbreaker at all, but I'd be happy to write something like this if you'd support it 😄

xotahal commented 4 years ago

Guys, just one more question about your approach. Just wondering - don't really want to start the discussion about what is better.

Untitled_Artwork

So usually you are moving your beta branch (triggers beta release) more often than your master branch (triggers production release). That means the commit range is different. Therefore it produces a different version number for the same commit in the git history. How do/would you deal with this?

jhbertra commented 4 years ago

The way I handle this is with pre release variants. My beta version tags for that range would look something like this

A: ios/5.4.1-beta1 B: ios/5.5.0-beta1 C: ios/5.5.0-beta2

E: ios/5.4.1 F: ios/5.5.0

Basically, for pre-release - determine what the version number would be for a production release, and if there are matching pre release variants between current commit and last production release, increment pre-release counter (or reset if different - e.g. beta3 to rc1). Then production releases pretty much ignore any pre release versions since last release.

hanno-jonlay commented 4 years ago

On my end, I don't consider the release process to be...

Release ios/alpha/5.4.1
Test ios/alpha/5.4.1
Once happy, produce ios/prod/5.4.1

Instead, I allow the releases to have a totally different numbering scheme:

Automatically release ios/alpha/5.4.1 via nightly build on develop branch
Test and verify
If satisfied, merge commits from develop => master
Nightly release runs on master branch with a totally different numbering scheme ios/prod/1.2.3

This is the same way that the android version of the app we're producing might be something totally different: android/alpha/10.6.2

Works for our scenario (releasing different versions of apps cross platform, with automated nightlies).

We have a totally different TestFlight app ID for our alpha releases. Works fine so far.

xotahal commented 4 years ago

As I said, there are as many opinions as there are people! :D

I don't believe we can cover all programmers around the world by providing more and more parameters to this plugin. And probably I don't even want to do that.

I think a better way would be to create better documentation about how this plugin handles increasing the version number (https://semver.org/) by using https://conventionalcommits.org

And if anyone wants to use another approach they need to find a different plugin.

Ideally, this plugin could be just for react-native (iOS, Android). It could handle the whole process of the configuration by typing just fastlane semantic-release init react-native --codepush. A programmer would provide usernames, passwords, etc.

hanno-jonlay commented 4 years ago

🤷‍♂ it's your call, ultimately.

I would personally see value in being able to configure the versioning "style". I can see some people raising eyebrows when the plugin is used for releasing public versions of a product and it jumps from v1.0.2 to v5.3.2 (example) in a couple of commits. I could foresee us hitting this issue sometime in the future. Maybe at that point I would personally have a stronger opinion about it, and contemplate a fork for our own purposes.

But that bit isn't a big issue for me right now - certainly not something I would argue passionately about 😆

Would definitely like to have a lock_version parameter though, if you'd consider accepting a PR for that. It would avoid that slightly hacky Fastfile I shared above. Won't bother with that if you don't think it adds value.

xotahal commented 4 years ago

This is a good read about this topic I think - http://sentimentalversioning.org/ :)

skimi commented 4 years ago

Coming from https://github.com/semantic-release/semantic-release or https://github.com/conventional-changelog/conventional-changelog the version increment is always minimal, grouping all changes to the closest major, minor or patch version.

Imo aligning with existing libraries makes sense to get consistent results across projects. I want my web-app changelog using conventional-changelog or sematic-release to look the same as my react-native one to avoid explaining to anyone why one changelog bumps 10 versions at a time and the other only one.