diff --git a/.github/workflows/milestone.yml b/.github/workflows/milestone.yml index 0086daa3eb..8d60419d11 100644 --- a/.github/workflows/milestone.yml +++ b/.github/workflows/milestone.yml @@ -6,9 +6,10 @@ name: Milestone # that have none. The rule is util/milestone.py's action_for, documented under # "Milestone" in docs/source/contributing/workflows.rst: an open item, a merged # pull request or an issue closed as completed gets the open milestone named -# after the checked-out `pcapkit.__version__`, unless it already holds another; -# an unmerged pull request or an issue closed as not planned or as a duplicate -# loses whichever milestone it holds. A missing milestone is a warning, never created. +# after the checked-out `pcapkit.__version__`, or the lowest open one above it +# once that one is closed (#2548), unless it already holds another; an unmerged +# pull request or an issue closed as not planned or as a duplicate loses +# whichever milestone it holds. A missing milestone is a warning, never created. # # SECURITY. `pull_request_target` gives this run a write token on events a # stranger can cause, so it must never execute anything the pull request diff --git a/docs/source/contributing/conventions/process.rst b/docs/source/contributing/conventions/process.rst index 5fbf5a626b..f56b839d09 100644 --- a/docs/source/contributing/conventions/process.rst +++ b/docs/source/contributing/conventions/process.rst @@ -354,9 +354,13 @@ request, an issue closed as not planned -- takes no milestone. :file:`.github/workflows/milestone.yml` gives an item the open milestone as it opens, and settles it as the item closes; a milestone already set by hand to another release is kept, and a manual run backfills the items without one -(:doc:`../workflows`). The closed milestones -were backfilled from merge and close dates against the release tags, so an item that -landed just before a tag may sit one line early. +(:doc:`../workflows`). **The milestone follows the release, not** ``__version__``: +after a final tag the version still names the release just made, so its closed +milestone gives way to the lowest open one above it. The owner ruled that on +:issue:`2548` over bumping ``__version__`` to a development release after each tag, +which would block the scheduled releases until the suffix came off by hand. The closed +milestones were backfilled from merge and close dates against the release tags, so an +item that landed just before a tag may sit one line early. **The project board records state that the labels drop.** The state labels above are stripped when an item closes, so they say only what is live. The `PyPCAPKit project diff --git a/docs/source/contributing/workflows.rst b/docs/source/contributing/workflows.rst index 520654e75b..7ae2fb3b46 100644 --- a/docs/source/contributing/workflows.rst +++ b/docs/source/contributing/workflows.rst @@ -597,7 +597,9 @@ closes, by the rule under "Milestones and the Project Board" in * an open item, a merged pull request, or an issue closed as completed gets the open milestone titled ``PyPCAPKit v.`` from the checked-out - ``__version__`` -- ``1.5.0b14`` gives ``PyPCAPKit v1.5``; + ``__version__`` -- ``1.5.0b14`` gives ``PyPCAPKit v1.5`` -- or, once that one is + closed, the open ``PyPCAPKit v.`` with the lowest release above it, + compared as numbers so ``v1.10`` follows ``v1.9`` (:issue:`2548`); * a pull request closed unmerged, or an issue closed as not planned or as a duplicate, loses any milestone it has; * an issue closed with no reason recorded is left as it is. @@ -609,15 +611,15 @@ the exception: an unmerged or not-planned close takes away whichever milestone t item holds, since work closed without shipping belongs to no release. The run re-reads the item through the API rather than trusting the event payload. -Where no open milestone has the title, the item is left without one and the run -emits a ``::warning::``: the script never creates a milestone, since opening the -next one is a release step done by hand. A removal needs no milestone, so it goes -ahead regardless. +Where no milestone has the title, or it is closed and none above it is open, the +item is left without one and the run emits a ``::warning::``: the script never +creates a milestone, since opening the next one is a release step done by hand. A +removal needs no milestone, so it goes ahead regardless. A ``workflow_dispatch`` backfills: it applies the same rule to every item, open or closed, with no milestone whose number is above the highest-numbered item of the previous release's milestone, the newest of the ``PyPCAPKit v.`` -milestones below the current one. Items at or below that bound belong to releases +milestones below the one it gives. Items at or below that bound belong to releases already made, and those among them without a milestone were left so by hand. With no earlier milestone to bound it, the backfill warns and changes nothing. Its ``dry_run`` input is ticked by default, so a dispatch reports what it would change diff --git a/tests/project/test_milestone_2218.py b/tests/project/test_milestone_2218.py index d45f14a0d0..87ff16bcb9 100644 --- a/tests/project/test_milestone_2218.py +++ b/tests/project/test_milestone_2218.py @@ -3,6 +3,8 @@ * :class:`TestTitle` and :class:`TestReadVersion` -- the milestone a version maps to, and the version read from :file:`pcapkit/__init__.py` as text. +* :class:`TestTarget` -- the milestone given: the version's own while it is open, + else the lowest open one above it, by number (#2548). * :class:`TestActionFor` -- the rule: an open item, a merged pull request or an issue closed as completed gets the milestone; an unmerged one or one closed as not planned loses it; an issue closed with no reason is left alone. @@ -155,6 +157,33 @@ def test_each_case(self) -> None: V16 = {'number': 12, 'title': 'PyPCAPKit v1.6', 'state': 'open'} V013 = {'number': 1, 'title': 'PyPCAPKit v0.13', 'state': 'closed'} OTHER = {'number': 11, 'title': 'Backlog', 'state': 'open'} +#: v1.5 after its final tag, while ``__version__`` still names it (#2548). +V15_CLOSED = dict(V15, state='closed') +V17 = {'number': 13, 'title': 'PyPCAPKit v1.7', 'state': 'open'} +V19_CLOSED = {'number': 20, 'title': 'PyPCAPKit v1.9', 'state': 'closed'} +V110 = {'number': 21, 'title': 'PyPCAPKit v1.10', 'state': 'open'} +V111 = {'number': 22, 'title': 'PyPCAPKit v1.11', 'state': 'open'} +V20 = {'number': 23, 'title': 'PyPCAPKit v2.0', 'state': 'open'} + + +class TestTarget(unittest.TestCase): + """#2548: the version's own milestone while it is open, else the lowest open one above it.""" + + def test_each_case(self) -> None: + cases = [ + ('own open', '1.5.1', (V14, V15, V16, OTHER), V15), + ('own open, an earlier one open too', '1.5.1', (dict(V14, state='open'), V15, V16), V15), + ('own closed', '1.5.1', (V013, V14, V15_CLOSED, V16, OTHER), V16), + ('own closed, the lowest later one', '1.5.1', (V15_CLOSED, V17, V16), V16), + ('own closed, a later one closed too', '1.5.1', (V15_CLOSED, dict(V16, state='closed'), V17), V17), + ('own closed, only an earlier one open', '1.5.1', (dict(V14, state='open'), V15_CLOSED, OTHER), None), + ('own closed, none later', '1.5.1', (V14, V15_CLOSED, OTHER), None), + ('own absent, a later one open', '1.5.1', (V14, V16, OTHER), None), + ('v1.10 follows v1.9', '1.9.2', (V20, V111, V19_CLOSED, V110), V110), + ] + for case, version, found, want in cases: + with self.subTest(case=case): + self.assertEqual(milestone.target_milestone(list(found), version), want) class FakeRest: @@ -203,8 +232,11 @@ def setUp(self) -> None: tmp = tempfile.TemporaryDirectory(prefix='pcapkit-2218-') # pylint: disable=consider-using-with self.addCleanup(tmp.cleanup) self.version = os.path.join(tmp.name, '__init__.py') + self.write_version('1.5.0b14') + + def write_version(self, version: 'str') -> None: with open(self.version, 'w', encoding='utf-8') as file: - file.write("__version__ = '1.5.0b14'\n") + file.write(f"__version__ = {version!r}\n") def run_main(self, argv: 'list[str]', fake: 'FakeRest') -> 'tuple[int, str]': #: The waits the run asked for, in order; nothing here really sleeps. @@ -278,19 +310,40 @@ def test_a_reopened_item_gets_it_back_after_a_removal(self) -> None: self.assertEqual(fake.patches, [(372, None), (372, 10)]) def test_a_missing_milestone_warns_and_changes_nothing(self) -> None: - for case, found in (('absent', (V013, V14, OTHER)), - ('closed', (V013, V14, dict(V15, state='closed')))): + for case, found, why in ( + ('absent', (V013, V14, OTHER), + "no open milestone is titled 'PyPCAPKit v1.5', so no item was given one; create it by hand."), + ('closed, none later', (V013, V14, V15_CLOSED, OTHER), + "'PyPCAPKit v1.5' is closed and no later release milestone is open, so no item was given one; " + 'create the next one by hand.')): with self.subTest(case=case): fake = FakeRest([pull(2205, merged=True)], found=found) code, out = self.run_main(['--item', '2205'], fake) self.assertEqual(code, 0) self.assertEqual(fake.patches, []) - self.assertEqual(out, "::warning title=Milestone::no open milestone is titled 'PyPCAPKit v1.5', " - 'so no item was given one; create it by hand.\n0 of 1 item(s) changed\n') + self.assertEqual(out, f'::warning title=Milestone::{why}\n0 of 1 item(s) changed\n') + + def test_a_closed_milestone_gives_the_next_open_one(self) -> None: + # #2548: 1.5.1 after v1.5's final tag, so its items go to v1.6. + self.write_version('1.5.1') + for case, item in (('issue opened', issue(2519, state='open', reason=None)), + ('merged pull request', pull(2519, merged=True))): + with self.subTest(case=case): + fake = FakeRest([item], found=(V013, V14, V15_CLOSED, V17, V16, OTHER)) + code, out = self.run_main(['--item', '2519'], fake) + self.assertEqual(code, 0) + self.assertEqual(fake.patches, [(2519, 12)]) + self.assertEqual(out, "#2519: set milestone to 'PyPCAPKit v1.6'\n1 of 1 item(s) changed\n") + + def test_the_next_release_is_found_numerically(self) -> None: + self.write_version('1.9.2') + fake = FakeRest([issue(900, state='open', reason=None)], found=(V20, V19_CLOSED, V111, V110)) + _, out = self.run_main(['--item', '900'], fake) + self.assertEqual(fake.patches, [(900, 21)]) + self.assertIn("#900: set milestone to 'PyPCAPKit v1.10'", out) def test_the_title_follows_the_version_file(self) -> None: - with open(self.version, 'w', encoding='utf-8') as file: - file.write("__version__ = '1.4.2'\n") + self.write_version('1.4.2') fake = FakeRest([issue(300)], found=(dict(V14, state='open'), V15)) self.run_main(['--item', '300'], fake) self.assertEqual(fake.patches, [(300, 9)]) @@ -364,6 +417,22 @@ def test_a_missing_milestone_warns_and_changes_nothing(self) -> None: self.assertEqual(fake.patches, []) self.assertIn("::warning title=Milestone::no open milestone is titled 'PyPCAPKit v1.5'", out) + def test_a_closed_milestone_bounds_at_its_own_items(self) -> None: + # #2548: v1.6 is given, so v1.5 is the release before it, and #400 below + # v1.5's newest item was left without a milestone on purpose. + self.write_version('1.5.1') + items = [issue(400), issue(2501), pull(2502, merged=True), issue(2503, current=V16), + issue(2504, state='open', reason=None)] + fake = FakeRest(items, found=(V013, V14, V15_CLOSED, V16, OTHER), newest={9: 336, 10: 2500, 1: 12}) + code, out = self.run_main(['--backfill'], fake) + self.assertEqual(code, 0) + self.assertEqual(fake.patches, [(2501, 12), (2502, 12), (2504, 12)]) + self.assertEqual(out.splitlines(), ['backfilling items above #2500 without a milestone', + "#2501: set milestone to 'PyPCAPKit v1.6'", + "#2502: set milestone to 'PyPCAPKit v1.6'", + "#2504: set milestone to 'PyPCAPKit v1.6'", + '3 of 3 item(s) changed']) + class TestPrecedence(_Runs): """A milestone already on an item wins over the rule's, on open and on close, but not over a removal.""" diff --git a/util/milestone.py b/util/milestone.py index d8d275c0ef..66cdb15b73 100644 --- a/util/milestone.py +++ b/util/milestone.py @@ -6,7 +6,8 @@ * an open item, a merged pull request, or an issue closed as completed gets the open milestone titled after the checked-out ``pcapkit.__version__``, - ``PyPCAPKit v.`` (:func:`milestone_title`); + ``PyPCAPKit v.`` (:func:`milestone_title`), or, where that one is + closed, the open release milestone lowest above it (:func:`target_milestone`); * a pull request closed unmerged, or an issue closed as not planned or as a duplicate, loses any milestone it has, whichever that is; * an issue closed with no reason recorded is left as it is. @@ -16,9 +17,9 @@ The version is read from :file:`pcapkit/__init__.py` as text (:func:`read_version`), so nothing from :mod:`pcapkit` is imported or run. Where the milestone for that -version is missing or closed, an item that would get it is left alone and a -``::warning::`` says so: creating the next milestone is a release step, done by -hand. +version is missing, or closed with no later one open, an item that would get it is +left alone and a ``::warning::`` says so: creating the next milestone is a release +step, done by hand. It runs in one of these modes: @@ -27,7 +28,7 @@ is re-read through the API rather than taken from the event payload, so a run applies the rule to the item as it stands when the run starts. * ``--backfill`` -- every item, open or closed, with no milestone whose number is - above the highest-numbered item of the previous release's milestone + above the highest-numbered item of the release milestone before the one given (:func:`backfill_bound`), for ``workflow_dispatch``. Items at or below that bound belong to releases already made, and the ones among them without a milestone were left without one on purpose. @@ -247,9 +248,24 @@ def milestones(rest: 'Rest', repo: 'str') -> 'list[dict[str, Any]]': return list(pages(rest, f'repos/{repo}/milestones?state=all')) -def find_open(found: 'list[dict[str, Any]]', title: 'str') -> 'Optional[dict[str, Any]]': - """The open milestone titled ``title`` among ``found``, if there is one.""" - return next((entry for entry in found if entry['title'] == title and entry['state'] == 'open'), None) +def target_milestone(found: 'list[dict[str, Any]]', version: 'str') -> 'Optional[dict[str, Any]]': + """The open milestone among ``found`` that a ``version``'s items get, if there is one. + + It is the one :func:`milestone_title` names. Where that one is closed, its + release has shipped while ``__version__`` still names it, so it is the open + release milestone with the lowest release above it, compared as numbers so that + ``v1.10`` follows ``v1.9`` (#2548). A version with no milestone at all gets none. + + """ + title = milestone_title(version) + own = [entry for entry in found if entry['title'] == title] + current = next((entry for entry in own if entry['state'] == 'open'), None) + if current is not None or not own: + return current + floor = release(version) + later = [(number, entry) for entry in found if entry['state'] == 'open' + for number in [title_release(entry['title'])] if number is not None and number > floor] + return min(later, key=lambda pair: pair[0])[1] if later else None def backfill_bound(rest: 'Rest', repo: 'str', found: 'list[dict[str, Any]]', @@ -336,21 +352,28 @@ def main(argv: 'Optional[list[str]]' = None, rest: 'Rest' = gh_rest, items = [rest('GET', f'repos/{args.repo}/issues/{args.item}', None)] actions = [action_for(items[0])] found = milestones(rest, args.repo) if SET in actions else [] + milestone = target_milestone(found, version) else: found = milestones(rest, args.repo) - bound = backfill_bound(rest, args.repo, found, release(version)) + milestone = target_milestone(found, version) + # Bounded by the release before the milestone given, not before the version's. + given = title if milestone is None else milestone['title'] + bound = backfill_bound(rest, args.repo, found, release(given[len(PREFIX):])) if bound is None: - _actions.annotate('warning', f'no release milestone before {title!r}, so nothing bounds the ' + _actions.annotate('warning', f'no release milestone before {given!r}, so nothing bounds the ' 'backfill; no item was changed.', title=ANNOTATION) return 0 print(f'backfilling items above #{bound} without a milestone') items = unmilestoned(rest, args.repo, bound) actions = [action_for(item) for item in items] - milestone = find_open(found, title) if milestone is None and SET in actions: - _actions.annotate('warning', f'no open milestone is titled {title!r}, so no item was given one; ' - 'create it by hand.', title=ANNOTATION) + if any(entry['title'] == title for entry in found): + message = (f'{title!r} is closed and no later release milestone is open, so no item was given one; ' + 'create the next one by hand.') + else: + message = f'no open milestone is titled {title!r}, so no item was given one; create it by hand.' + _actions.annotate('warning', message, title=ANNOTATION) wrote = False