SFDO-Tooling / CumulusCI

Python framework for building portable automation for Salesforce projects
http://cumulusci.readthedocs.io
BSD 3-Clause "New" or "Revised" License
363 stars 242 forks source link

Parse release branch versions using LooseVersion #3726

Open jlantz opened 10 months ago

jlantz commented 10 months ago

Currently release branch version numbers can be only an integer with the format feature/NNN. Using the LooseVersion parser from CumulusCI's utilities to parse the version number allows for greater flexibility in release branch naming.

This branch includes modified release branch parsing logic that uses LooseVersion to parse the version number instead of the current isdigit() check. It then checks if there are only integers in the parsed version number's parts and if so, returns the LooseVersion. This approach is backwards compatible with the previous int-only approach.

For example, if you have the following branches:

And you wanted to create a new planned patch release of 260 developed in parallel, you could simply create the branch feature/260.1

The branch includes doc updates and tests with full coverage of the changes.

TedHusted commented 10 months ago

@jlantz We support monthly releases, and for various reasons, we name our branches "release/v$YYYY-$MM", such as "release/v2024-01".

(1) Is there a reason why the matching is not based on a regular expression or some pattern that could be configurable?

Just asking in case you already considered and dismissed the idea.

-- Off-Topic --

In our case, we include the "v" to support an integration with another system that does not support numeric symbols.

If I found a workaround for that issue, we could also go with "2024.01".

I do find it a bit curious that release branches use a feature prefix. We could change that too, but referring to the release branches in this way seems more natural.

For the GitHub tag, we also use release/* -- and here is where we use "release/4.1" for example.

(2) Is avoiding name collisions a driver for using "feature" for the release branches?

We tend to avoid prefixes for our work branches, and just name them for the Jira issues (ABC-123) without a prefix.

(3) I'm not clear on the value of prefixes generally, and I sometimes wonder if they are an example of over-design.

jlantz commented 10 months ago

That's not a bad idea about parsing with a regex, but I'd consider that a separate feature than this one. If you wanted to implement regex without this feature, you'd need a regex that produces an integer. The reason is that CumulusCI has logic to automatically merge to newer release branches so it needs an integer to do a comparison. This PR just changes that logic to parse that integer string with LooseVersion so it can include .'s and sorts the way you'd expect version numbers (or any string of numbers separated by dots) to sort.

Together, this PR and your proposal for regex would mean the regex can produce either a string parsable as an integer or a version number by LooseVersion.

jlantz commented 10 months ago

@TedHusted As for the off-topic portion, I just went through this whole series of discussion and debate with one of my clients over the course of ~2 months. I'll try my best to recall the arguments that led to our decision to stick with feature/ for release feature branches instead of release/...

In this context, having main, the default branch, represent the next release (N) made the most sense since that represents the main development effort of the team. Whenever a dev wants to start a new branch, they create it from the default branch. By comparison, if N were release/N instead of main, all new branches must be created against whatever branch represents the current N branch. You can create those as child branches like release/N__some-feature, but then release branches are already behaving like feature branches. You could create feature/some-feature from the release/N branch, but consider the scenario where you're working on N and N+1 in parallel. Looking at a feature branch, you'd have to look into the commit history to see which release branch it was created from.

With main + feature/N+1 formatting, all branch names tell you what they're against. If they're a feature in the main line of development on N, they're simple feature branches. If they're for an N+n release, they start with feature/[N+n version]__some-feature making it obvious from the branch name that they're related to the release. This branching hierarchy is also used by CumulusCI for child branch merge downs.

jlantz commented 1 month ago

@jstvz OK, comments resolved and ready for re-review

jstvz commented 1 month ago

@jlantz LGTM. Needs prettier/isort though.