Skip to content

Commit

Permalink
Parse release branch versions using LooseVersion and still support
Browse files Browse the repository at this point in the history
int-only formatting correctly
  • Loading branch information
jlantz committed Jan 2, 2024
1 parent 359425d commit 35082a0
Show file tree
Hide file tree
Showing 6 changed files with 179 additions and 48 deletions.
3 changes: 1 addition & 2 deletions cumulusci/core/dependencies/resolvers.py
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,7 @@ def get_branches(

class GitHubReleaseBranchCommitStatusResolver(AbstractGitHubReleaseBranchResolver):
"""Resolver that identifies a ref by finding a beta 2GP package version
in a commit status on a `feature/NNN` release branch."""
in a commit status on a `feature/NNN` release branch."""

name = "GitHub Release Branch Commit Status Resolver"
commit_status_context = "2gp_context"
Expand Down Expand Up @@ -545,7 +545,6 @@ def dependency_filter_ignore_deps(ignore_deps: List[dict]) -> Callable:
ignore_namespace = [d["namespace"] for d in ignore_deps if "namespace" in d]

def should_include(some_dep: Dependency) -> bool:

if isinstance(some_dep, PackageNamespaceVersionDependency):
return some_dep.namespace not in ignore_namespace
if isinstance(some_dep, BaseGitHubDependency):
Expand Down
13 changes: 9 additions & 4 deletions cumulusci/tasks/github/merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from cumulusci.core.utils import process_bool_arg
from cumulusci.tasks.github.base import BaseGithubTask
from cumulusci.utils.git import is_release_branch
from cumulusci.utils.version_strings import LooseVersion


class MergeBranch(BaseGithubTask):
Expand Down Expand Up @@ -195,7 +196,7 @@ def _get_branches_to_merge(self):
return to_merge

def _get_next_release(self, repo_branches):
"""Returns the integer that corresponds to the lowest release number found on all release branches.
"""Returns the version number that corresponds to the lowest release number found on all release branches.
NOTE: We assume that once a release branch is merged that it will be deleted.
"""
release_nums = [
Expand Down Expand Up @@ -231,12 +232,14 @@ def _is_release_branch(self, branch_name):
"""A release branch begins with the given prefix"""
return is_release_branch(branch_name, self.options["branch_prefix"])

def _get_release_number(self, branch_name) -> int:
def _get_release_number(self, branch_name) -> LooseVersion:
"""Get the release number from a release branch name.
Assumes we already know it is a release branch.
"""
return int(branch_name.split(self.options["branch_prefix"])[1])
version = branch_name.split(self.options["branch_prefix"])[1]
version = LooseVersion(version)
return version

def _merge(self, branch_name, source, commit):
"""Attempt to merge a commit from source to branch with branch_name"""
Expand Down Expand Up @@ -305,4 +308,6 @@ def _is_future_release_branch(self, branch_name, next_release):
def _get_release_num(self, release_branch_name):
"""Given a release branch, returns an integer that
corresponds to the release number for that branch"""
return int(release_branch_name.split(self.options["branch_prefix"])[1])
version = release_branch_name.split(self.options["branch_prefix"])[1]
version = LooseVersion(version)
return version
115 changes: 96 additions & 19 deletions cumulusci/tasks/github/tests/test_merge.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,11 @@
from cumulusci.tasks.github import MergeBranch
from cumulusci.tasks.release_notes.tests.utils import MockUtilBase
from cumulusci.tests.util import DummyOrgConfig, create_project_config
from cumulusci.utils.version_strings import LooseVersion


class TestMergeBranch(MockUtilBase):
def setup_method(self):

# Set up the mock values
self.repo = "TestRepo"
self.owner = "TestOwner"
Expand Down Expand Up @@ -616,6 +616,47 @@ def test_branches_to_merge__main_to_feature_and_next_release(self):
assert expected_branches_to_merge == actual_branches
assert 2 == len(responses.calls)

@responses.activate
def test_branches_to_merge__main_to_feature_and_next_release_version_number(self):
"""Tests that when main branch is the source_branch
that all expected child branches and the *lowest numbered*
release branch using a version number are merged into."""

self._setup_mocks(
[
"main",
"feature/2.3",
"feature/3.4.0",
"feature/450",
"feature/work-a",
"feature/work-b",
"feature/work-a__child_a",
"feature/work-a__child_a__grandchild",
"feature/work-b__child_b",
"feature/orphan__with_child",
"feature/230__cool_feature",
"feature/230__cool_feature__child",
]
)

task = self._create_task(
task_config={
"options": {
"source_branch": "main",
}
}
)
task._init_task()

actual_branches = [branch.name for branch in task._get_branches_to_merge()]
expected_branches_to_merge = [
"feature/2.3",
"feature/work-a",
"feature/work-b",
]
assert expected_branches_to_merge == actual_branches
assert 2 == len(responses.calls)

@responses.activate
def test_branches_to_merge__no_prefix_merge_to_feature(self):
"""Tests that when source_branch is a branch other than main
Expand Down Expand Up @@ -758,6 +799,35 @@ def test_merge_to_future_release_branches(self):
assert ["feature/232", "feature/300"] == actual_branches
assert 2 == len(responses.calls)

@responses.activate
def test_merge_to_future_release_branches_version_number(self):
"""Tests that commits to the main branch are merged to the expected feature branches"""
self._setup_mocks(
[
"main",
"feature/2.3",
"feature/2.4",
"feature/300",
"feature/work-item",
]
)

task = self._create_task(
task_config={
"options": {
"source_branch": "feature/2.3",
"branch_prefix": "feature/",
"update_future_releases": True,
}
}
)
task._init_task()

actual_branches = [branch.name for branch in task._get_branches_to_merge()]

assert ["feature/2.4", "feature/300"] == actual_branches
assert 2 == len(responses.calls)

@responses.activate
def test_merge_to_future_release_branches_skip(self):
"""Tests that commits to the main branch are merged to the expected feature branches"""
Expand Down Expand Up @@ -1016,15 +1086,21 @@ def test_is_release_branch(self):
f"{prefix}20302",
f"{prefix}3810102",
f"{prefix}9711112",
f"{prefix}0.1",
f"{prefix}1.0",
f"{prefix}1.2.3.456.789",
]
invalid_release_branches = [
f"{prefix}200_",
f"{prefix}_200" f"{prefix}230_",
f"{prefix}230__child",
f"{prefix}230__grand__child",
f"{prefix}230a",
f"{prefix}2.3.0__child",
f"{prefix}2.3.0__grand__child",
f"{prefix}r1",
f"{prefix}R1",
f"{prefix}foo",
]
task = self._create_task(
task_config={
Expand Down Expand Up @@ -1069,7 +1145,7 @@ def test_set_next_release(self):
task._init_task()

repo_branches = list(task.repo.branches())
assert task._get_next_release(repo_branches) == 88
assert task._get_next_release(repo_branches) == LooseVersion("88")

@responses.activate
def test_is_future_release_branch(self):
Expand All @@ -1090,23 +1166,24 @@ def test_is_future_release_branch(self):
task._init_task()

repo_branches = list(task.repo.branches())
assert task._get_next_release(repo_branches) == 8

assert not task._is_future_release_branch("f", 8)
assert not task._is_future_release_branch("feature", 8)
assert not task._is_future_release_branch("feature/", 8)
assert not task._is_future_release_branch("feature/_", 8)
assert not task._is_future_release_branch("feature/0", 8)
assert not task._is_future_release_branch("feature/O", 8)
assert not task._is_future_release_branch("feature/7", 8)
assert not task._is_future_release_branch("feature/8", 8)
assert not task._is_future_release_branch("feature/9_", 8)

assert task._is_future_release_branch("feature/9", 8)
assert task._is_future_release_branch("feature/75", 8)
assert task._is_future_release_branch("feature/123", 8)
assert task._is_future_release_branch("feature/4567", 8)
assert task._is_future_release_branch("feature/10000", 8)
expected_next_version = LooseVersion("8")
assert task._get_next_release(repo_branches) == expected_next_version

assert not task._is_future_release_branch("f", expected_next_version)
assert not task._is_future_release_branch("feature", expected_next_version)
assert not task._is_future_release_branch("feature/", expected_next_version)
assert not task._is_future_release_branch("feature/_", expected_next_version)
assert not task._is_future_release_branch("feature/0", expected_next_version)
assert not task._is_future_release_branch("feature/O", expected_next_version)
assert not task._is_future_release_branch("feature/7", expected_next_version)
assert not task._is_future_release_branch("feature/8", expected_next_version)
assert not task._is_future_release_branch("feature/9_", expected_next_version)

assert task._is_future_release_branch("feature/9", expected_next_version)
assert task._is_future_release_branch("feature/75", expected_next_version)
assert task._is_future_release_branch("feature/123", expected_next_version)
assert task._is_future_release_branch("feature/4567", expected_next_version)
assert task._is_future_release_branch("feature/10000", expected_next_version)


def log_header():
Expand Down
21 changes: 19 additions & 2 deletions cumulusci/utils/git.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import pathlib
import re
from typing import Any, Optional, Tuple
from cumulusci.utils.version_strings import LooseVersion

EMPTY_URL_MESSAGE = """
The provided URL is empty or no URL under git remote "origin".
Expand Down Expand Up @@ -33,14 +34,30 @@ def is_release_branch(branch_name: str, prefix: str) -> bool:
if not branch_name.startswith(prefix):
return False
parts = branch_name[len(prefix) :].split("__")
return len(parts) == 1 and parts[0].isdigit()
if not parts[0]:
return False
version = LooseVersion(parts[0])
is_version = True
for part in version.version:
if isinstance(part, str):
is_version = False
break
return len(parts) == 1 and is_version


def is_release_branch_or_child(branch_name: str, prefix: str) -> bool:
if not branch_name.startswith(prefix):
return False
parts = branch_name[len(prefix) :].split("__")
return len(parts) >= 1 and parts[0].isdigit()
if not parts[0]:
return False
version = LooseVersion(parts[0])
is_version = True
for part in version.version:
if isinstance(part, str):
is_version = False
break
return len(parts) >= 1 and is_version


def get_feature_branch_name(branch_name: str, prefix: str) -> Optional[str]:
Expand Down
47 changes: 40 additions & 7 deletions docs/cumulusci-flow.md
Original file line number Diff line number Diff line change
Expand Up @@ -134,18 +134,18 @@ default flows:
Some teams deliver large releases several times a year. For this type of
release cadence, Salesforce.org uses a special type of branch referred
to as a release branch. Release branches are simply a feature branch
named with a number. These long-lived branches are created off of the
`main` branch, serve as the target branch for all features associated
with that release and are eventually merged back to the `main` branch
when a release occurs. To be able to clearly track what work is
associated with a specific release, release branches must fulfill these
criteria:
named with a number or a version number. These long-lived branches are
created off of the `main` branch, serve as the target branch for all
features associated with that release and are eventually merged back to
the `main` branch when a release occurs. To be able to clearly track what
work is associated with a specific release, release branches must fulfill
these criteria:

- They are the parent branches of _all_ feature work associated with a
release. That is, all feature branches associated with a release are
child branches of the target release branch.
- Release branches use a strict naming format: `feature/release_num`
where `release_num` is a valid integer.
where `release_num` is a valid integer or a version number made up of valid integers separated by `.`.

Using the `feature/` branch prefix for the release branch names allow
those branches to stay in sync with the `main` branch. Like any other
Expand Down Expand Up @@ -332,6 +332,39 @@ convention described above. Commits **never** propagate in the opposite
direction. (A commit to `feature/002` would never be merged to
`feature/001` if it was an existing branch in the GitHub repository).

Consider the following branches using version numbers in a GitHub repository:

- `main` - Source of Truth for Production
- `feature/9.25` - The next major production release
- `feature/9.25__feature1` - A single feature associated with release
`002`
- `feature/9.25__large_feature` - A large feature associated with
release `9.25`
- `feature/9.25__large_feature__child1` - First chunk of work for the
large feature
- `feature/9.25__large_feature__child2` - Second chunk of work for the
large feature
- `feature/10.0` - The release that comes after `9.25`
- `feature/10.0__feature1` - A single feature associated with release
`10.3`

This scenario illustrates the different sort ordering applied when using
the version number format for release branch names. If you used only
integers for the version numbers, `925` would sort after `100`. Using the
version number format of a series of integers separated by `.`, the sorting
is applied as you would expect for a version number where `9.25` is less
than `10.0`.

In this scenario, CumulusCI ensures that when `feature/9.25` receives a
commit, that that commit is also merged into `feature/10.0`. This kicks
off tests in our CI system and ensures that functionality going into
`feature/9.25` doesn't break work being done for future releases. Once
those tests pass, the commit on `feature/10.0` is merged to
`feature/9.25__feature1` because they adhere to the parent/child naming
convention described above. Commits **never** propagate in the opposite
direction. (A commit to `feature/9.25` would never be merged to
`feature/9.24` if it was an existing branch in the GitHub repository).

**Propagating commits to future release branches is turned off by
default.** If you would like to enable this feature for your GitHub
repository, you can set the `update_future_releases` option on the
Expand Down
28 changes: 14 additions & 14 deletions docs/dev.md
Original file line number Diff line number Diff line change
Expand Up @@ -653,22 +653,22 @@ utilize a release branch model and build second-generation package betas

> - If a `tag` is present, use the commit for that tag, and any
> package version found there. (Resolver: `tag`)
> - If the current branch is a release branch (`feature/NNN`, where
> `feature/` is the feature branch prefix and `NNN` is any integer)
> - If the current branch is a release branch (`feature/NNN` or `feature/N.N.N`, where
> `feature/` is the feature branch prefix and `NNN` is any integer or `N.N.N` is a version number made up of integers)
> or a child branch of a release branch, locate a branch with the
> same name in the dependency repository. If a commit status
> contains a beta package Id for any of the first five commits on
> that branch, use that commit and package. (Resolver:
> `commit_status_exact_branch`)
> - If the current branch is a release branch (`feature/NNN`, where
> `feature/` is the feature branch prefix and `NNN` is any integer)
> - If the current branch is a release branch (`feature/NNN` or `feature/N.N.N`, where
> `feature/` is the feature branch prefix and `NNN` is any integer or `N.N.N` is a version number made up of integers)
> or a child branch of a release branch, locate a matching release
> branch (`feature/NNN`) in the dependency repository. If a commit
> branch (`feature/NNN` or `feature/N.N.N`) in the dependency repository. If a commit
> status contains a beta package Id for any of the first five
> commits on that branch, use that commit and package. (Resolver:
> `commit_status_release_branch`)
> - If the current branch is a release branch (`feature/NNN`, where
> `feature/` is the feature branch prefix and `NNN` is any integer)
> - If the current branch is a release branch (`feature/NNN` or `feature/N.N.N`, where
> `feature/` is the feature branch prefix and `NNN` is any integer or `N.N.N` is a version number made up of integers)
> or a child branch of a release branch, locate a branch for either
> of the two previous releases (e.g., `feature/230` in this
> repository would search `feature/229` and `feature/228`) in the
Expand Down Expand Up @@ -696,22 +696,22 @@ utilize a release branch model and build unlocked package betas
also suitable for use cases where a persistent org and Unlocked
Package versions are used for ongoing QA.

> - If the current branch is a release branch (`feature/NNN`, where
> `feature/` is the feature branch prefix and `NNN` is any integer)
> - If the current branch is a release branch (`feature/NNN` or `feature/N.N.N`, where
> `feature/` is the feature branch prefix and `NNN` is any integer or `N.N.N` is a version number made up of integers)
> or a child branch of a release branch, locate a branch with the
> same name in the dependency repository. If a commit status
> contains a beta package Id for any of the first five commits on
> that branch, use that commit and package. (Resolver:
> `unlocked_exact_branch`)
> - If the current branch is a release branch (`feature/NNN`, where
> `feature/` is the feature branch prefix and `NNN` is any integer)
> - If the current branch is a release branch (`feature/NNN` or `feature/N.N.N`, where
> `feature/` is the feature branch prefix and `NNN` is any integer or `N.N.N` is a version number made up of integers)
> or a child branch of a release branch, locate a matching release
> branch (`feature/NNN`) in the dependency repository. If a commit
> branch (`feature/NNN` or `feature/N.N.N`) in the dependency repository. If a commit
> status contains a beta package Id for any of the first five
> commits on that branch, use that commit and package. (Resolver:
> `unlocked_release_branch`)
> - If the current branch is a release branch (`feature/NNN`, where
> `feature/` is the feature branch prefix and `NNN` is any integer)
> - If the current branch is a release branch (`feature/NNN` or `feature/N.N.N`, where
> `feature/` is the feature branch prefix and `NNN` is any integer or `N.N.N` is a version number made up of integers)
> or a child branch of a release branch, locate a branch for either
> of the two previous releases (e.g., `feature/230` in this
> repository would search `feature/229` and `feature/228`) in the
Expand Down

0 comments on commit 35082a0

Please sign in to comment.