Add branch/type filters to transitions and dict-based fixVersion mapping#460
Add branch/type filters to transitions and dict-based fixVersion mapping#460istein1 wants to merge 5 commits intorelease-engineering:mainfrom
Conversation
…rsion mapping, and tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…pe-filtered-transitions
|
/ok-to-test |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
Hi @istein1, this looks great.
However, I've got a few items for you. I've got a concern about one log message which I think might get noisy; I have a question for Ralph; and I have a tiny coding suggestion. The rest of my comments are on the tests.
Your initial coverage looks very good, but there are some cases which I think should have additional scenarios, even though they won't increase the coverage, because they represent valid use situations, so I think we should exercise them in the tests. Also, I recommend covering your new _matches_transition_filters() function as a separate unit (and maybe with the table-driven structure, if that works).
Lastly, I think this change should include a documentation update, since you're adding a couple of new configuration features. (Here, at the minimum.)
Thanks!
| if ( | ||
| closed_status is not True | ||
| and issue.status == "Closed" | ||
| and existing.fields.status.name.upper() != closed_status.upper() | ||
| ): |
There was a problem hiding this comment.
@ralphbean , what is it supposed to mean when closed_status is True (i.e., when the config key 'issue_updates' includes {'transition': True})? (As it happens, nobody appears to do this except release-engineering/Sync2Jira....)
The documentation is less than clear, and my interpretation of it doesn't match my interpretation of this code.... (Should that be "if closed_status is True or the existing status doesn't match"?)
True is an acceptable value, but False is not...which is weird! And, if we're the only ones using this, now might be a good time to rescind the practice. (I think that True is supposed to be equivalent to "Closed".)
|
Thank you @webbnh, |
- Guard noisy log in update_transition when pr_updates is absent - Simplify base_branch extraction with dict reference instead of .get() - Split PR filter tests into separate units for _matches_transition_filters and update_transition - Add missing issue test scenarios: absent/empty issue_updates, none-match multiple entries, already-matching status, transition=True - Fix misleading "cumulative" comment in issue test docstring - Document branches, issue_types filters and dict-based fixVersion mapping Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Replace `closed_status is not True` with `isinstance(closed_status, str)` to safely skip any non-string value (True, False, None, etc.) - Show branches/issue_types as keys in the same dict in pr_updates docs - Add test scenario for transition: False Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@webbnh ,Thanks for the detailed feedback! |
|
/ok-to-test |
webbnh
left a comment
webbnh
left a comment
There was a problem hiding this comment.
The code looks good; I just have a couple of thoughts on the doc changes.
| * :code:`{'merge_transition': 'MODIFIED', 'branches': ['release-*', 'main'], 'issue_types': ['Bug', 'Story']}` | ||
| * Optional filters added to the same dict as the transition. ``branches`` accepts glob patterns and restricts | ||
| the transition to PRs whose target branch matches. ``issue_types`` restricts it to matching downstream JIRA | ||
| issue types. Both filters are optional and can be used independently or together. |
There was a problem hiding this comment.
We probably shouldn't have the 'link_transition' between the two 'merge_transition' descriptions.
And, I would be inclined to have just one 'merge_transition' entry, perhaps like
* :code:`{'merge_transition': 'MODIFIED', 'branches': ['release-*', 'main'], 'issue_types': ['Bug', 'Story']}`
* Sync when upstream PR gets merged. Attempts to transition JIRA ticket to CUSTOM_TRANSITION on upstream merge.
* ``branches`` and ``issue_types`` are optional and either may be omitted. ``branches`` accepts glob patterns
and restricts the transition to PRs whose target branch matches. ``issue_types`` restricts it to matching
downstream JIRA issue types.If you concur, I would recommend the same for issue_updates (lines 170-173).
| with a mapping (for our example it would be 'Test milestone'). | ||
| * :code:`{'fixVersion': {'0.9.0': 'Product 8.1', '1.0.0': 'Product 9.0'}}` | ||
| * Dict lookup format. Maps specific upstream milestones to specific downstream fixVersions. | ||
| Milestones not present in the dict are left unchanged. |
There was a problem hiding this comment.
You should probably tweak lines 206-207 as well. E.g.,
* You can add the following to the mapping array. This array will map an upstream field to the downstream counterpart
using either a template or a mapping table.2 participants
Summary
branches(glob) andissue_typesfilters to PR transition configs, so transitions only fire for matching branches and Jira issue typesissue_typesfilter support to issue transition configsfixVersionmapping (in addition to existing string template format)base_branchfield to PR intermediary, extracted from GitHub PR payloadTest plan
_update_transitionwith issue_types filtering (7 scenarios)map_fixVersion(string template + dict lookup)Issue.from_github🤖 Generated with Claude Code