Skip to content

feat(parse-label-filter): decide skip from which field the edit touched - #211

Open
sydorovdmytro wants to merge 2 commits into
mainfrom
devops-1057/parse-label-filter-edit-fields
Open

feat(parse-label-filter): decide skip from which field the edit touched#211
sydorovdmytro wants to merge 2 commits into
mainfrom
devops-1057/parse-label-filter-edit-fields

Conversation

@sydorovdmytro

Copy link
Copy Markdown
Contributor

What

Adds optional body-changed and base-changed inputs so skip-edited is
decided from which field an edited event touched, instead of being inferred
from a body comparison that cannot see the difference.

Why

skip-edited inferred "the body before the edit" from
github.event.changes.body.from. That key is absent unless the body changed,
so every non-body edited event arrived with an empty previous body — which
parses as "the label-filter block was removed".

vcluster-pro's PR template ships a pre-filled block:

​```label-filter
none
​```

so for the default PR shape a title-only edit compares none against "",
reports a changed filter, and re-runs the suite for nothing. With the DEVOPS-1057
concurrency guard also in place, that run queues behind the in-flight one
rather than replacing it — the exact downgrade the guard and skip-edited are
paired to prevent. Found reviewing loft-sh/vcluster-pro#2156.

Comparing bodies cannot fix this, because the information is not in the bodies.

Approach

edited carrying verdict why
changes.title only skip title, labels, assignees cannot change a filter that lives in the body
changes.body compare the blocks unchanged from today
changes.base never skip a different base can mean a different thing under test
  body-changed: ${{ github.event.changes.body != null }}
  base-changed: ${{ github.event.changes.base != null }}

Why two inputs and not one

Both were tried as one and are wrong:

  • body-changed alone reports a base retarget as skippable, since a retarget
    carries no body change either. A caller resolving an OSS branch from
    base_ref would then never test the new pairing. This was caught by Bugbot on
    the caller-side attempt in vcluster-pro#2156.
  • changes.body.from || pull_request.body in the caller silences a body that
    genuinely changed from empty — a PR that just gained a label-filter block,
    which must run.

Both are now tests, so neither can be reintroduced.

Compatibility

Both inputs are optional and default to the legacy body comparison, so this needs
no coordinated rollout across the four vcluster-pro branches that call it. A
regression test pins the legacy misread, so the cost of omitting them is explicit
rather than folklore.

Only the exact strings "true"/"false" are acted on — a caller expression that
renders oddly falls back to comparing rather than producing a silent skip.

Tests

make test-parse-label-filter — 21 pass (8 new). shellcheck clean, docs
regenerated, make check-docs clean.

Follow-up

Wiring the two inputs into the callers (main, v0.34, v0.35, v0.36 of
vcluster-pro) once parse-label-filter/v1 is repointed. Not urgent — omitting
them is exactly today's behaviour.

Part of DEVOPS-1057.

skip-edited compared the label-filter block before and after the edit, and
inferred "before" from github.event.changes.body.from. That key is absent unless
the BODY changed, so every non-body `edited` event arrived with an empty previous
body — which parses as "the block was removed".

For a PR following vcluster-pro's template, whose label-filter block is
pre-filled with `none`, a title-only edit therefore compares "none" against ""
and reports a changed filter. The suite re-runs for nothing, and once the caller
also has the DEVOPS-1057 concurrency guard that run queues behind the in-flight
one instead of replacing it. Found reviewing vcluster-pro#2156.

Comparing bodies cannot fix this, because the information is not in the bodies.
Take it from the payload instead: body-changed and base-changed, each wired from
a null test on the changes key.

    changes.title only -> skip. Title, labels, assignees and milestones cannot
                          change a filter that lives in the body.
    changes.body       -> compare the blocks, as before.
    changes.base       -> never skip. A different base can mean a different thing
                          under test, and the previous result does not carry over.

Both inputs, not one. A single body-changed boolean reports a base retarget as
skippable, since a retarget carries no body change either — that was the first
fix attempted in the caller and Bugbot caught it there. A caller-side
`changes.body.from || pull_request.body` is also wrong for a different reason: it
silences a body that genuinely changed FROM empty, i.e. a PR that just gained a
label-filter block, which must run. Both cases are now tests.

Optional, defaulting to the legacy comparison, so this needs no coordinated
rollout across the four vcluster-pro branches. A regression test pins the legacy
misread so the cost of omitting them is explicit rather than folklore, and only
the exact strings "true"/"false" are acted on so a caller expression that renders
oddly falls back to comparing rather than silently skipping.

Part of DEVOPS-1057
…iptions

The composite smoke test caught it: the runner parses expressions in an action
manifest, and a literal ${{ github... }} anywhere in it — including an input
description — is rejected with "Unrecognized named-value: 'github'", which fails
the whole action load rather than just that field.

    ##[error].../parse-label-filter/action.yml (Line: 29, Col: 18):
      Unrecognized named-value: 'github'
    ##[error]Failed to load .../parse-label-filter/action.yml

Describe the inputs in prose instead and leave the caller expressions to the
README, with a comment on the inputs so the next person does not reintroduce it.
Checked the other actions: no other description carries a literal expression.

actionlint does not flag this, which is why the smoke test is the thing that
found it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant