From: Simon Glass <simon.glass@canonical.com> Add automatic database updates when pickman MRs are merged. The step command now: 1. Checks for merged pickman MRs via GitLab API 2. Parses the MR description to find the source branch and last commit 3. Updates the database's last_commit for the source branch 4. Then proceeds to check for open MRs and create new ones as before This removes the need to manually run commit-source after each MR is merged, enabling fully automated cherry-pick workflows with poll. Add get_merged_pickman_mrs() and refactor get_open_pickman_mrs() to use a common get_pickman_mrs() function with a state parameter. Co-developed-by: Claude Opus 4.5 <noreply@anthropic.com> --- tools/pickman/README.rst | 13 +++-- tools/pickman/control.py | 94 ++++++++++++++++++++++++++++++++++++- tools/pickman/ftest.py | 74 ++++++++++++++++++++++++++--- tools/pickman/gitlab_api.py | 38 +++++++++++++-- 4 files changed, 203 insertions(+), 16 deletions(-) diff --git a/tools/pickman/README.rst b/tools/pickman/README.rst index 7a33378a32b..07b9408afa0 100644 --- a/tools/pickman/README.rst +++ b/tools/pickman/README.rst @@ -117,9 +117,16 @@ To automatically create an MR if none is pending:: ./tools/pickman/pickman step us/next -This checks for open pickman MRs (those with ``[pickman]`` in the title) and if -none exist, runs ``apply`` with ``--push`` to create a new one. This is useful -for automated workflows where only one MR should be active at a time. +This command performs the following: + +1. Checks for merged pickman MRs and updates the database with the last + cherry-picked commit from each merged MR +2. Checks for open pickman MRs (those with ``[pickman]`` in the title) +3. If no open MRs exist, runs ``apply`` with ``--push`` to create a new one + +This is useful for automated workflows where only one MR should be active at a +time. The automatic database update on merge means you don't need to manually +run ``commit-source`` after each MR is merged. Options for the step command: diff --git a/tools/pickman/control.py b/tools/pickman/control.py index 6efb20df1d5..aaed06c1e35 100644 --- a/tools/pickman/control.py +++ b/tools/pickman/control.py @@ -513,11 +513,95 @@ def do_review(args, dbs): # pylint: disable=unused-argument return 0 +def parse_mr_description(description): + """Parse a pickman MR description to extract source and last commit + + Args: + description (str): MR description text + + Returns: + tuple: (source_branch, last_commit_hash) or (None, None) if not parseable + """ + import re + + # Extract source branch from "## date: source_branch" line + source_match = re.search(r'^## [^:]+: (.+)$', description, re.MULTILINE) + if not source_match: + return None, None + source = source_match.group(1) + + # Extract commits from "- hash subject" lines + commit_matches = re.findall(r'^- ([a-f0-9]+) ', description, re.MULTILINE) + if not commit_matches: + return None, None + + # Last commit is the last one in the list + last_hash = commit_matches[-1] + + return source, last_hash + + +def process_merged_mrs(remote, source, dbs): + """Check for merged MRs and update the database + + Args: + remote (str): Remote name + source (str): Source branch name to filter by + dbs (Database): Database instance + + Returns: + int: Number of MRs processed, or -1 on error + """ + merged_mrs = gitlab_api.get_merged_pickman_mrs(remote) + if merged_mrs is None: + return -1 + + processed = 0 + for merge_req in merged_mrs: + mr_source, last_hash = parse_mr_description(merge_req['description']) + + # Only process MRs for the requested source branch + if mr_source != source: + continue + + # Check if this MR's last commit is newer than current database + current = dbs.source_get(source) + if not current: + continue + + # Resolve the short hash to full hash + try: + full_hash = run_git(['rev-parse', last_hash]) + except Exception: # pylint: disable=broad-except + tout.warning(f"Could not resolve commit '{last_hash}' from " + f"MR !{merge_req['iid']}") + continue + + # Check if this commit is newer than current (current is ancestor of it) + try: + # Is current an ancestor of last_hash? (meaning last_hash is newer) + run_git(['merge-base', '--is-ancestor', current, full_hash]) + except Exception: # pylint: disable=broad-except + continue # current is not an ancestor, so last_hash is not newer + + # Update database + short_old = current[:12] + short_new = full_hash[:12] + tout.info(f"MR !{merge_req['iid']} merged, updating '{source}': " + f'{short_old} -> {short_new}') + dbs.source_set(source, full_hash) + dbs.commit() + processed += 1 + + return processed + + def do_step(args, dbs): """Create an MR if none is pending - Checks for open pickman MRs and if none exist, runs apply with push - to create a new one. + Checks for merged pickman MRs and updates the database, then checks for + open pickman MRs and if none exist, runs apply with push to create a new + one. Args: args (Namespace): Parsed arguments with 'source', 'remote', 'target' @@ -527,6 +611,12 @@ def do_step(args, dbs): int: 0 on success, 1 on failure """ remote = args.remote + source = args.source + + # First check for merged MRs and update database + processed = process_merged_mrs(remote, source, dbs) + if processed < 0: + return 1 # Check for open pickman MRs mrs = gitlab_api.get_open_pickman_mrs(remote) diff --git a/tools/pickman/ftest.py b/tools/pickman/ftest.py index 252fb79d2be..e5ed187bdf6 100644 --- a/tools/pickman/ftest.py +++ b/tools/pickman/ftest.py @@ -1126,6 +1126,54 @@ class TestParseStep(unittest.TestCase): self.assertEqual(args.target, 'main') +class TestParseMrDescription(unittest.TestCase): + """Tests for parse_mr_description function.""" + + def test_parse_mr_description(self): + """Test parsing a valid MR description.""" + description = """## 2025-01-15: us/next + +Branch: cherry-abc123 + +Commits: +- abc123a First commit +- def456b Second commit +- caf789c Third commit + +### Conversation log +Some log text""" + source, last_hash = control.parse_mr_description(description) + self.assertEqual(source, 'us/next') + self.assertEqual(last_hash, 'caf789c') + + def test_parse_mr_description_single_commit(self): + """Test parsing MR description with single commit.""" + description = """## 2025-01-15: feature/branch + +Branch: cherry-xyz + +Commits: +- abc1234 Only commit""" + source, last_hash = control.parse_mr_description(description) + self.assertEqual(source, 'feature/branch') + self.assertEqual(last_hash, 'abc1234') + + def test_parse_mr_description_invalid(self): + """Test parsing invalid MR description.""" + source, last_hash = control.parse_mr_description('invalid description') + self.assertIsNone(source) + self.assertIsNone(last_hash) + + def test_parse_mr_description_no_commits(self): + """Test parsing MR description without commits.""" + description = """## 2025-01-15: us/next + +Branch: cherry-abc""" + source, last_hash = control.parse_mr_description(description) + self.assertIsNone(source) + self.assertIsNone(last_hash) + + class TestStep(unittest.TestCase): """Tests for step command.""" @@ -1136,17 +1184,19 @@ class TestStep(unittest.TestCase): 'title': '[pickman] Test MR', 'web_url': 'https://gitlab.com/mr/123', } - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', - return_value=[mock_mr]): - args = argparse.Namespace(cmd='step', source='us/next', - remote='ci', target='master') - ret = control.do_step(args, None) + with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + return_value=[]): + with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + return_value=[mock_mr]): + args = argparse.Namespace(cmd='step', source='us/next', + remote='ci', target='master') + ret = control.do_step(args, None) self.assertEqual(ret, 0) def test_step_gitlab_error(self): """Test step when GitLab API returns error.""" - with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', return_value=None): args = argparse.Namespace(cmd='step', source='us/next', remote='ci', target='master') @@ -1154,6 +1204,18 @@ class TestStep(unittest.TestCase): self.assertEqual(ret, 1) + def test_step_open_mrs_error(self): + """Test step when get_open_pickman_mrs returns error.""" + with mock.patch.object(gitlab_api, 'get_merged_pickman_mrs', + return_value=[]): + with mock.patch.object(gitlab_api, 'get_open_pickman_mrs', + return_value=None): + args = argparse.Namespace(cmd='step', source='us/next', + remote='ci', target='master') + ret = control.do_step(args, None) + + self.assertEqual(ret, 1) + class TestParseReview(unittest.TestCase): """Tests for review command argument parsing.""" diff --git a/tools/pickman/gitlab_api.py b/tools/pickman/gitlab_api.py index ca239c41271..6720e0a1526 100644 --- a/tools/pickman/gitlab_api.py +++ b/tools/pickman/gitlab_api.py @@ -152,15 +152,16 @@ def create_mr(host, proj_path, source, target, title, desc=''): return None -def get_open_pickman_mrs(remote): - """Get open merge requests created by pickman +def get_pickman_mrs(remote, state='opened'): + """Get merge requests created by pickman Args: remote (str): Remote name + state (str): MR state ('opened', 'merged', 'closed', 'all') Returns: - list: List of dicts with 'iid', 'title', 'web_url', 'source_branch' keys, - or None on failure + list: List of dicts with 'iid', 'title', 'web_url', 'source_branch', + 'description' keys, or None on failure """ if not check_available(): return None @@ -181,7 +182,7 @@ def get_open_pickman_mrs(remote): glab = gitlab.Gitlab(f'https://{host}', private_token=token) project = glab.projects.get(proj_path) - mrs = project.mergerequests.list(state='opened', get_all=True) + mrs = project.mergerequests.list(state=state, get_all=True) pickman_mrs = [] for merge_req in mrs: if '[pickman]' in merge_req.title: @@ -190,6 +191,7 @@ def get_open_pickman_mrs(remote): 'title': merge_req.title, 'web_url': merge_req.web_url, 'source_branch': merge_req.source_branch, + 'description': merge_req.description or '', }) return pickman_mrs except gitlab.exceptions.GitlabError as exc: @@ -197,6 +199,32 @@ def get_open_pickman_mrs(remote): return None +def get_open_pickman_mrs(remote): + """Get open merge requests created by pickman + + Args: + remote (str): Remote name + + Returns: + list: List of dicts with 'iid', 'title', 'web_url', 'source_branch' keys, + or None on failure + """ + return get_pickman_mrs(remote, state='opened') + + +def get_merged_pickman_mrs(remote): + """Get merged merge requests created by pickman + + Args: + remote (str): Remote name + + Returns: + list: List of dicts with 'iid', 'title', 'web_url', 'source_branch', + 'description' keys, or None on failure + """ + return get_pickman_mrs(remote, state='merged') + + def get_mr_comments(remote, mr_iid): """Get human comments on a merge request (excluding bot/system notes) -- 2.43.0