Skip to content

fix(ci): read backport_branch from the top-level key only, and honour an empty value - #2552

Open
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:ci-release-notes-backport-branch
Open

fix(ci): read backport_branch from the top-level key only, and honour an empty value#2552
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:ci-release-notes-backport-branch

Conversation

@LeSingh1

@LeSingh1 LeSingh1 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Stacking note: #2496 also touches ci/tools/check_release_notes.py and its test file (the os.pathpathlib migration, part 7 of #2410). That PR rewrites the path = os.path.join(...) line in this same function; this one changes the loop body and the regex. Adjacent but disjoint — flagging it so a conflict here is expected rather than surprising.

Problem

load_backport_branch reads backport_branch out of ci/versions.yml with a regex, then falls back to GITHUB_REF_NAME:

BACKPORT_BRANCH_RE = re.compile(r"""^backport_branch:\s*["']?(?P<branch>[^"'\s#]+)""")
...
for line in f:
    m = BACKPORT_BRANCH_RE.match(line.strip())
    if m:
        return m.group("branch")
...
github_ref_name = os.environ.get("GITHUB_REF_NAME", "")
if BACKPORT_BRANCH_NAME_RE.match(github_ref_name):
    return github_ref_name

1. An explicitly empty value is treated as an absent key. The capture is +, so it needs at least one character. backport_branch: "" matches nothing, which is indistinguishable from the key not being there — and control falls through to the environment.

2. .strip() erases the indentation the anchor depends on. The regex is anchored at column 0 precisely so it matches the top-level key. Stripping first makes any nesting level match, and the first one in file order wins.

Measured against the real load_backport_branch on main:

ci/versions.yml GITHUB_REF_NAME result on main should be
backport_branch: "12.9.x" unset '12.9.x'
backport_branch: "12.9.x" 11.8.x '12.9.x'
backport_branch: "" unset None None
backport_branch: "" 12.9.x '12.9.x' None
nested backport_branch: "11.8.x" above top-level "12.9.x" unset '11.8.x' '12.9.x'
# backport_branch: "9.9.x" above "12.9.x" unset '12.9.x'

Row 4 is the one that bites: a maintainer who blanks backport_branch to disable backport gating gets it silently re-enabled from whatever branch the workflow happened to run on. backport_branch feeds validate_backport_decision, so this decides whether a release is allowed to proceed without backport notes.

Fix

  • Match the raw line, so only a column-0 backport_branch: counts. (The anchor was already there; it just could not do its job after .strip().)
  • Let the capture be *, and return None for an empty value without consulting the environment — an explicit blank is a decision, not a missing key.

Everything already working is unchanged: a quoted value still wins over GITHUB_REF_NAME, an absent file still falls back to it, a non-backport GITHUB_REF_NAME is still ignored, and a commented-out key is still skipped.

Tests

TestLoadBackportBranch covered only three cases (quoted-non-empty, file-absent + env set, file-absent + env main). Added:

  • an explicitly empty value with GITHUB_REF_NAME set — the silent-override case;
  • a configured value with a different GITHUB_REF_NAME, pinning precedence;
  • a nested backport_branch above the top-level one;
  • a commented-out key above the real one.

The new tests monkeypatch.delenv("GITHUB_REF_NAME", raising=False) where the environment must not participate; the existing tests do not, so they were order-sensitive.

Verification

Executed in full (stdlib only, no GPU, no network):

# with the fix
pytest ci/tools/tests  ->  60 passed

# with ci/tools/check_release_notes.py restored from upstream/main
FAILED TestLoadBackportBranch::test_explicitly_empty_value_is_not_overridden_by_the_environment
FAILED TestLoadBackportBranch::test_nested_key_does_not_shadow_the_top_level_one
2 failed, 44 passed

ruff check / ruff format --check clean on both files. Restore done with cp aside + git show upstream/main:<path> >; index verified clean before committing.

…our an empty value

`load_backport_branch` reads `backport_branch` out of ci/versions.yml with a
regex, then falls back to GITHUB_REF_NAME. Two things go wrong.

1. The value capture is `[^"'\s#]+`, requiring at least one character. An
   explicitly empty `backport_branch: ""` therefore matches nothing, which is
   indistinguishable from an absent key, so control falls through to the
   environment:

       backport_branch: ""   + GITHUB_REF_NAME=12.9.x  ->  '12.9.x'

   A maintainer who blanks the setting to disable backport gating gets it
   silently re-enabled from whatever branch the workflow ran on.

2. The line is `.strip()`ed before matching, which erases indentation. The
   regex is anchored at column 0 precisely so it matches a top-level key, but
   after stripping any nesting level matches, and the first one in file order
   wins:

       cuda:
         legacy:
           backport_branch: "11.8.x"
       backport_branch: "12.9.x"      ->  '11.8.x'

Match the raw line so only a column-0 key counts, and let the capture be
empty so an explicit empty value returns None without consulting the
environment.

A commented-out `# backport_branch:` was already ignored correctly; a test
now pins that too.
@copy-pr-bot

copy-pr-bot Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the CI/CD CI/CD infrastructure label Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CD CI/CD infrastructure

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant