geido opened a new pull request, #33180:
URL: https://github.com/apache/superset/pull/33180
### SUMMARY
A chart's `controlPanel` declares the `form_data` keys that get persisted on
every slice using it, and its `transformProps` reads those keys back at render
time. Changing either can silently break charts and dashboards that were saved
against the previous shape. We have no metadata versioning today, so nothing
surfaces that risk during review.
The original version of this PR added a dedicated workflow that failed CI
unless someone applied a `validation:backward-compatible` label. @rusackas
pointed out we don't need a whole new action for that, since the repo already
runs `actions/labeler` — so this is just a few globs in `.github/labeler.yml`.
That's what this PR does now: **64 lines of workflow replaced by a 12-line
config entry.** Touching chart metadata applies `risk:chart-metadata`, sitting
alongside the `risk:db-migration` and `risk:ci-script` warnings already in that
file.
Proper metadata versioning is still the real fix; this is the cheap
mitigation in the meantime.
#### ⚠️ This flags, it does not enforce
Worth being explicit, since the earlier revision was approved as a blocking
check: the labeler only applies a label. It cannot fail CI, and nothing stops a
PR merging with the label on it. The intent is to put the risk in front of the
reviewer, not to gate it.
I'd rather land the visibility on its own than bolt a second workflow on to
recreate the gate. If we do want teeth later, `no-hold-label.yml` is already
the pattern for it — a small `pull_request` job that reads the PR's labels and
calls `core.setFailed()` — and it can be added on top of this without changing
these globs.
#### Coverage
Deliberately not limited to `superset-frontend/plugins`. Native filters,
chart customizations and TimeTable all register through the same plugin API
from `superset-frontend/src`, and native filter config is persisted into
dashboard `json_metadata`, so it carries the same risk:
| Glob | Matches |
| --- | --- |
| `superset-frontend/**/{controlPanel,transformProps}.{ts,tsx}` | 125 files
— every chart plugin at any nesting depth, native filters, chart customizations
|
| `superset-frontend/**/config/{controlPanel,transformProps}/index.ts` |
TimeTable's config barrels |
| `superset-frontend/src/explore/controlPanels/**` | shared control panel
sections |
131 files in total. The globs match metadata files by exact name, so unit
tests like `test/transformProps.test.ts` don't trigger the label — the one
exception is `explore/controlPanels/Separator.test.ts`, which sits in a
directory that is otherwise entirely metadata.
Known gap: `plugin-chart-cartodiagram`'s `controlPanelUtil.tsx` /
`transformPropsUtil.ts` aren't matched. Catching those needs a prefix glob,
which drags in ~64 test files. Not worth the noise for one plugin's helpers —
the label is a prompt for review, not a proof.
#### Before merging
`risk:chart-metadata` doesn't exist yet. `actions/labeler` will auto-create
it, but as an undescribed grey label, out of step with the rest of the `risk:`
family (`#e67e22` + a description). Happy to create it properly — or rename it,
if `viz:` is the better home given the `viz:charts:xyz` TODO at the top of this
file. Just say which.
```
gh label create "risk:chart-metadata" --repo apache/superset --color e67e22 \
--description "PR changes a chart's controlPanel/transformProps; needs
backward-compatibility review"
```
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A — CI config only, no UI change.
### TESTING INSTRUCTIONS
`actions/labeler` runs on `pull_request_target`, so the new entry only takes
effect once this is on `master` — it can't label this PR itself. To verify the
globs without merging, run them through `minimatch`, which is what the pinned
action uses internally:
```bash
npm i minimatch js-yaml
node -e "
const {Minimatch}=require('minimatch'), yaml=require('js-yaml'),
fs=require('fs');
const
globs=yaml.load(fs.readFileSync('.github/labeler.yml','utf8'))['risk:chart-metadata'][0]['changed-files'][0]['any-glob-to-any-file'];
const ms=globs.map(g=>new Minimatch(g,{dot:true}));
require('child_process').execSync('git
ls-files').toString().trim().split('\n')
.filter(f=>ms.some(m=>m.match(f))).forEach(f=>console.log(f));
"
```
Expect 131 files, no `.test.` files other than `Separator.test.ts`. Spot
checks:
-
`superset-frontend/plugins/plugin-chart-echarts/src/Timeseries/Regular/Line/controlPanel.tsx`
→ labeled
- `superset-frontend/src/filters/components/Select/controlPanel.ts` → labeled
-
`superset-frontend/plugins/plugin-chart-country-map/test/transformProps.test.ts`
→ not labeled
- `superset-frontend/src/explore/components/ControlPanelsContainer.tsx` →
not labeled
After merge, open any PR touching a plugin's `controlPanel` and confirm the
label lands.
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in
[SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
