Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 34 additions & 0 deletions dev/breeze/src/airflow_breeze/commands/ci_commands.py
Original file line number Diff line number Diff line change
Expand Up @@ -1285,6 +1285,40 @@ def set_milestone(
console_print(f"[error]Failed to check existing milestone: {e}[/]")
return

# Re-read labels from the freshly-fetched issue to close the race between
# the workflow's initial label snapshot (taken by the get-pr-info job a
# couple of minutes ago) and the actual milestone-set step. Maintainers
# sometimes add and remove a backport label inside that window; honour
# the latest state, not the stale snapshot.
try:
current_labels = [label.name for label in issue.labels]
except Exception as e:
console_print(f"[warning]Could not re-read PR labels; falling back to snapshot decision: {e}[/]")
current_labels = labels

if set(current_labels) != set(labels):
console_print("[info]Labels changed since workflow snapshot; re-evaluating.[/]")
console_print(f"[info]Snapshot labels: {sorted(labels)}[/]")
console_print(f"[info]Current labels: {sorted(current_labels)}[/]")

if _should_skip_milestone_tagging(current_labels):
Copy link
Copy Markdown
Member

@jason810496 jason810496 May 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand the event correctly, we should add additional check in _should_skip_milestone_tagging.
If there's any event that maintainer remove any backport label themself. We need to skip the auto tagging.

Even with the new if set(current_labels) != set(labels): check, I don't think the existing implementation of _should_skip_milestone_tagging would be enough.

# Labels that indicate the PR should be skipped from milestone auto-tagging
MILESTONE_SKIP_LABELS: frozenset[str] = frozenset({"area:dev-tools", "area:dev-env", "area:CI"})

def _should_skip_milestone_tagging(labels: list[str]) -> bool:
"""Check if the PR should be skipped from milestone auto-tagging."""
return bool(set(labels) & MILESTONE_SKIP_LABELS)

Btw, I can help follow-up for this issue if you don't have bandwidth at this moment.

console_print(
f"[info]Skipping milestone tagging - PR now has skip label(s): "
f"{set(current_labels) & MILESTONE_SKIP_LABELS}[/]"
)
return

new_version, new_reason = _determine_milestone_version(current_labels, pr_title, base_branch)
if (new_version, new_reason) != (version, reason):
console_print(
f"[info]Determination changed after re-read: was ({version}, {reason!r}); "
f"now ({new_version}, {new_reason!r}). Using current labels.[/]"
)
version, reason = new_version, new_reason
if version is None:
console_print(f"[info]No milestone to set after re-evaluation: {reason}[/]")
return

major, minor = version
milestone_prefix = _get_milestone_prefix(major, minor)
console_print(f"[info]Looking for milestone with prefix: {milestone_prefix}[/]")
Expand Down
147 changes: 147 additions & 0 deletions dev/breeze/tests/test_set_milestone.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,13 @@
)


def _label(name: str) -> MagicMock:
"""Build a mock that quacks like a PyGithub ``Label`` for ``issue.labels``."""
m = MagicMock()
m.name = name
return m


class TestParseVersionFromBranch:
"""Test cases for _parse_version_from_branch."""

Expand Down Expand Up @@ -420,6 +427,8 @@ def test_find_milestone_should_set_and_comment(

mock_gh, mock_repo, mock_issue = mock_github_setup
mock_issue.milestone = None
# Fresh-issue labels match the workflow snapshot — no race, no re-evaluation.
mock_issue.labels = [_label(name) for name in pr_labels]
mock_milestone = MagicMock()
mock_milestone.title = milestone_title
mock_milestone.number = 42
Expand Down Expand Up @@ -530,6 +539,8 @@ def test_not_find_milestone_should_comment_warning(

mock_gh, mock_repo, mock_issue = mock_github_setup
mock_issue.milestone = None
# Fresh-issue labels match the workflow snapshot — no race, no re-evaluation.
mock_issue.labels = [_label(name) for name in pr_labels]
captured_comments: list[str] = []
mock_issue.create_comment.side_effect = lambda c: captured_comments.append(c)

Expand Down Expand Up @@ -571,3 +582,139 @@ def test_not_find_milestone_should_comment_warning(
"""
assert captured_comments[0] == expected_comment
assert "No open milestone found" in result.output

@patch("airflow_breeze.commands.ci_commands._get_github_client")
def test_backport_label_removed_after_snapshot_should_skip(
self, mock_get_client, cli_runner, mock_github_setup
):
"""If a backport label is removed between the workflow snapshot and the action,
the action must re-read labels from the issue and honour the current state —
skip milestone-set when the only signal that triggered it (the backport label)
is gone. Regression test for PR #67301 race.
"""
from airflow_breeze.commands.ci_commands import ci_group

mock_gh, mock_repo, mock_issue = mock_github_setup
mock_issue.milestone = None
mock_issue.labels = [_label("kind:documentation")]
mock_get_client.return_value = mock_gh

result = cli_runner.invoke(
ci_group,
[
"set-milestone",
"--pr-number",
"67301",
"--pr-title",
"fix: typo",
"--pr-labels",
json.dumps(["backport-to-v3-2-test", "kind:documentation"]),
"--base-branch",
"main",
"--merged-by",
"shahar1",
"--github-token",
"fake-token",
"--github-repository",
"apache/airflow",
],
)

# Snapshot still has the backport label, but the fresh issue.labels does not.
# The action must re-read, notice the change, and skip the milestone-set.
mock_issue.edit.assert_not_called()
mock_issue.create_comment.assert_not_called()
assert "Labels changed since workflow snapshot" in result.output
assert "No milestone to set after re-evaluation" in result.output
assert result.exit_code == 0

@patch("airflow_breeze.commands.ci_commands._get_github_client")
def test_backport_label_changed_after_snapshot_should_use_current(
self, mock_get_client, cli_runner, mock_github_setup
):
"""If the backport label is replaced with a different version between
snapshot and action (e.g. someone fixes the version target), the action
must re-determine the version using the current label, not the stale one.
"""
from airflow_breeze.commands.ci_commands import ci_group

mock_gh, mock_repo, mock_issue = mock_github_setup
mock_issue.milestone = None
# Fresh state: now targets v3-2-test, not v3-1-test.
mock_issue.labels = [_label("backport-to-v3-2-test"), _label("kind:bug")]
mock_milestone = MagicMock()
mock_milestone.title = "Airflow 3.2.3"
mock_milestone.number = 140
mock_get_client.return_value = mock_gh
mock_repo.get_milestones.return_value = [mock_milestone]

captured_comments: list[str] = []
mock_issue.create_comment.side_effect = lambda c: captured_comments.append(c)

result = cli_runner.invoke(
ci_group,
[
"set-milestone",
"--pr-number",
"12345",
"--pr-title",
"Fix: scheduler issue",
"--pr-labels",
json.dumps(["backport-to-v3-1-test", "kind:bug"]),
"--base-branch",
"main",
"--merged-by",
"testuser",
"--github-token",
"fake-token",
"--github-repository",
"apache/airflow",
],
)

mock_issue.edit.assert_called_once_with(milestone=mock_milestone)
assert "Labels changed since workflow snapshot" in result.output
assert "Determination changed after re-read" in result.output
assert "Airflow 3.2.3" in captured_comments[0]
assert "backport label targeting v3-2-test" in captured_comments[0]
assert result.exit_code == 0

@patch("airflow_breeze.commands.ci_commands._get_github_client")
def test_skip_label_added_after_snapshot_should_skip(
self, mock_get_client, cli_runner, mock_github_setup
):
"""A skip label added after the snapshot must also halt the action."""
from airflow_breeze.commands.ci_commands import ci_group

mock_gh, mock_repo, mock_issue = mock_github_setup
mock_issue.milestone = None
# Snapshot had no skip label; fresh state added area:CI.
mock_issue.labels = [_label("backport-to-v3-1-test"), _label("area:CI")]
mock_get_client.return_value = mock_gh

result = cli_runner.invoke(
ci_group,
[
"set-milestone",
"--pr-number",
"12345",
"--pr-title",
"CI tweak",
"--pr-labels",
json.dumps(["backport-to-v3-1-test"]),
"--base-branch",
"main",
"--merged-by",
"testuser",
"--github-token",
"fake-token",
"--github-repository",
"apache/airflow",
],
)

mock_issue.edit.assert_not_called()
mock_issue.create_comment.assert_not_called()
assert "Skipping milestone tagging" in result.output
assert "area:CI" in result.output
assert result.exit_code == 0
Loading