mirror of
https://github.com/BerriAI/litellm.git
synced 2026-09-10 22:41:41 +00:00
fix(ci): stop the name-collision check failing legal workflows
An expression at `jobs.<id>.strategy` is legal on GitHub, but the model required a mapping there, so a workflow using one made the whole file unreadable and turned code-quality red. That job's names are now a blind spot like any other name the sweep cannot work out offline. A matrix whose `name:` holds no matrix value publishes that one name once per combination, which leaves a required context just as ambiguous as two jobs sharing a name, so it now reports instead of deduping. A file that does not parse as one YAML document is reported the way the module already promised, rather than escaping as a traceback.
This commit is contained in:
parent
904542a559
commit
aa2c41f489
2 changed files with 94 additions and 16 deletions
|
|
@ -1,10 +1,12 @@
|
|||
"""Catch two workflow jobs that publish check runs under the same name.
|
||||
"""Catch workflow jobs that publish check runs under the same name.
|
||||
|
||||
A ruleset's required status check names a check run and GitHub matches it by that
|
||||
name alone. When two jobs publish the same name the required context stops
|
||||
mapping to the job that proves it: the commit carries two check runs under one
|
||||
name and nothing says which one the ruleset required. Both being green hides the
|
||||
clash completely, so the context quietly stops meaning what the ruleset intended.
|
||||
One job lands in the same place when its `name:` holds no matrix value, since
|
||||
every combination it runs then reports under that one name.
|
||||
|
||||
`.github/workflows/auto-close-duplicates.yml` shipped a job id `test` while
|
||||
`.github/workflows/test-mcp.yml` already published the required `test` context,
|
||||
|
|
@ -29,16 +31,17 @@ combination is filled in is one GitHub resolves per job, so it is one of those:
|
|||
guessing that two jobs sharing such a template clash would fail workflows over a
|
||||
context this sweep cannot read. A matrix that is itself an expression or that
|
||||
lists values which are not scalars, an `include` or `exclude` row shaped the same
|
||||
way, and a call this sweep cannot follow, go in the same bucket. The cost is that a real clash hiding behind one of them goes unseen,
|
||||
which leaves a merge no worse off than before this check existed, where the
|
||||
opposite direction would block work that was fine.
|
||||
way, a whole `strategy:` that comes from an expression, and a call this sweep
|
||||
cannot follow, go in the same bucket. The cost is that a real clash hiding behind
|
||||
one of them goes unseen, which leaves a merge no worse off than before this check
|
||||
existed, where the opposite direction would block work that was fine.
|
||||
|
||||
A job calling a local reusable workflow publishes one check run per job of the
|
||||
callee, named `<caller> / <callee>` and chained through however many levels of
|
||||
local calls it takes, which is why a caller's name never collides with a plain
|
||||
job that happens to match it. A file under `.github/workflows/` that does not read as
|
||||
a workflow at all is reported rather than skipped, since skipping it silently
|
||||
would hide every job it holds.
|
||||
job that happens to match it. A file under `.github/workflows/` that does not
|
||||
read as one workflow at all is reported rather than skipped, since skipping it
|
||||
silently would hide every job it holds.
|
||||
"""
|
||||
|
||||
import itertools
|
||||
|
|
@ -90,7 +93,7 @@ class Names:
|
|||
class Job(BaseModel):
|
||||
name: object = None
|
||||
uses: str | None = None
|
||||
strategy: Mapping[str, object] = Field(default_factory=dict)
|
||||
strategy: object = Field(default_factory=dict)
|
||||
|
||||
|
||||
class Workflow(BaseModel):
|
||||
|
|
@ -104,7 +107,10 @@ def scalar_text(value: object) -> str:
|
|||
|
||||
def parse(source: str) -> tuple[Workflow, object] | Unreadable:
|
||||
"""The workflow plus its raw `on:` value, or why the file does not read as one."""
|
||||
parsed: Final = yaml.safe_load(source)
|
||||
try:
|
||||
parsed: Final = yaml.safe_load(source)
|
||||
except yaml.YAMLError:
|
||||
return Unreadable("it does not read as one YAML document")
|
||||
if not isinstance(parsed, dict):
|
||||
return Unreadable("its top level is not a mapping of workflow keys")
|
||||
try:
|
||||
|
|
@ -186,6 +192,8 @@ def crossed_values(listed: Sequence[tuple[str, tuple[str, ...]]]) -> tuple[Mappi
|
|||
|
||||
def matrix_combinations(job: Job) -> tuple[Mapping[str, str], ...] | Opaque:
|
||||
"""One mapping per job the matrix produces, `exclude` applied before `include` as GitHub does."""
|
||||
if not isinstance(job.strategy, Mapping):
|
||||
return Opaque("its whole `strategy` comes from an expression")
|
||||
matrix: Final = job.strategy.get("matrix")
|
||||
if matrix is None:
|
||||
return ()
|
||||
|
|
@ -304,7 +312,7 @@ def expand(template: str, job: Job) -> Names:
|
|||
if isinstance(combinations, Opaque):
|
||||
return Names((), (combinations.reason,))
|
||||
over: Final = combinations or (NO_MATRIX,)
|
||||
return settled(tuple(dict.fromkeys(rendered(template, values) for values in over)))
|
||||
return settled(tuple(rendered(template, values) for values in over))
|
||||
|
||||
|
||||
def suffixed(job_id: str, combination: Mapping[str, str]) -> str:
|
||||
|
|
@ -408,13 +416,25 @@ def owners_by_name(sources: Mapping[str, str]) -> Iterator[tuple[str, tuple[str,
|
|||
yield name, tuple(owner for _, owner in pairs)
|
||||
|
||||
|
||||
def clash(name: str, owners: Sequence[str]) -> str | None:
|
||||
"""Why one name is ambiguous, whether two jobs carry it or one job repeats it over its matrix."""
|
||||
jobs: Final = tuple(dict.fromkeys(owners))
|
||||
if len(jobs) > 1:
|
||||
return (
|
||||
f"`{name}` is published by {len(jobs)} jobs: {', '.join(jobs)}. A required status check matching "
|
||||
f"that name cannot say which job proves it; give one of them a distinct `name:` or job id."
|
||||
)
|
||||
if len(owners) > 1:
|
||||
return (
|
||||
f"`{name}` is published {len(owners)} times by {jobs[0]}, once per matrix combination. A required "
|
||||
f"status check matching that name cannot say which run proves it; put a matrix value in its `name:`."
|
||||
)
|
||||
return None
|
||||
|
||||
|
||||
def collisions(sources: Mapping[str, str]) -> tuple[str, ...]:
|
||||
return tuple(
|
||||
f"`{name}` is published by {len(owners)} jobs: {', '.join(owners)}. A required status check matching "
|
||||
f"that name cannot say which job proves it; give one of them a distinct `name:` or job id."
|
||||
for name, owners in owners_by_name(sources)
|
||||
if len(owners) > 1
|
||||
)
|
||||
found: Final = tuple(clash(name, owners) for name, owners in owners_by_name(sources))
|
||||
return tuple(message for message in found if message is not None)
|
||||
|
||||
|
||||
def workflow_sources() -> Mapping[str, str]:
|
||||
|
|
|
|||
|
|
@ -699,3 +699,61 @@ def test_an_include_row_naming_a_listed_key_extends_only_the_combinations_it_mat
|
|||
|
||||
assert frozenset(name for name, _ in published(sources)) == frozenset({"3.12 fast"})
|
||||
assert len(blind_spots(sources)) == 1
|
||||
|
||||
|
||||
def test_a_job_whose_whole_strategy_is_an_expression_is_reported_rather_than_rejecting_the_file() -> None:
|
||||
sources: Final = {
|
||||
"plan.yml": (
|
||||
"on: pull_request\njobs:\n plan:\n name: Plan\n runs-on: ubuntu-latest\n"
|
||||
" fan:\n strategy: ${{ fromJSON(needs.plan.outputs.strategy) }}\n runs-on: ubuntu-latest\n"
|
||||
)
|
||||
}
|
||||
|
||||
assert unreadable(sources) == ()
|
||||
assert frozenset(name for name, _ in published(sources)) == frozenset({"Plan"})
|
||||
assert "`strategy` comes from an expression" in blind_spots(sources)[0]
|
||||
assert exit_code(sources) == 0
|
||||
|
||||
|
||||
def test_one_job_publishing_one_name_for_every_matrix_combination_is_a_collision() -> None:
|
||||
sources: Final = {
|
||||
"unit.yml": (
|
||||
"on: pull_request\njobs:\n build:\n name: Run tests\n runs-on: ubuntu-latest\n"
|
||||
' strategy:\n matrix:\n python-version: ["3.12", "3.13"]\n'
|
||||
)
|
||||
}
|
||||
|
||||
found: Final = collisions(sources)
|
||||
assert len(found) == 1
|
||||
assert "`Run tests` is published 2 times by unit.yml job `build`" in found[0]
|
||||
assert exit_code(sources) == 1
|
||||
|
||||
|
||||
def test_a_name_carrying_a_matrix_value_publishes_one_name_per_combination_without_colliding() -> None:
|
||||
sources: Final = {
|
||||
"unit.yml": (
|
||||
"on: pull_request\njobs:\n build:\n name: Run tests ${{ matrix.python-version }}\n"
|
||||
' runs-on: ubuntu-latest\n strategy:\n matrix:\n python-version: ["3.12", "3.13"]\n'
|
||||
)
|
||||
}
|
||||
|
||||
assert frozenset(name for name, _ in published(sources)) == frozenset({"Run tests 3.12", "Run tests 3.13"})
|
||||
assert collisions(sources) == ()
|
||||
assert exit_code(sources) == 0
|
||||
|
||||
|
||||
def test_a_file_that_is_not_valid_yaml_is_reported_rather_than_raising() -> None:
|
||||
sources: Final = {"broken.yml": "jobs:\n build: [\n"}
|
||||
|
||||
assert unreadable(sources) == (
|
||||
"broken.yml sits in the workflows directory but it does not read as one YAML "
|
||||
"document, so none of its jobs were checked.",
|
||||
)
|
||||
assert exit_code(sources) == 1
|
||||
|
||||
|
||||
def test_a_file_holding_two_yaml_documents_is_reported_rather_than_raising() -> None:
|
||||
sources: Final = {"two.yml": "on: pull_request\n---\non: push\n"}
|
||||
|
||||
assert len(unreadable(sources)) == 1
|
||||
assert exit_code(sources) == 1
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue