CritasWang commented on PR #118:
URL: https://github.com/apache/iotdb-extras/pull/118#issuecomment-5161630997
Thanks for taking this on — moving off the archived `@grafana/toolkit` is
the right direction, and the PR description is unusually thorough about its own
trade-offs (the externals/`grafanaDependency` reasoning and the self-disclosed
unfixed mutation sites are exactly the kind of thing that makes a migration
reviewable).
Three things below. The first is blocking; the second is a suggestion that
would have caught the first automatically; the third needs a call from the PMC
rather than from you or me.
---
## 1. Blocking: `connectors/grafana-plugin/pom.xml` needs to change in this
same PR
The PR description explains why `.github/workflows/grafana-plugin.yml` had
to be updated here — "it is what the migration breaks". That reasoning is
correct, but it applies equally to a second build path that this PR leaves
untouched.
`connectors/grafana-plugin/pom.xml` drives the frontend build through
`frontend-maven-plugin`, pinned to yarn:
```xml
<goal>install-node-and-yarn</goal>
<nodeVersion>v16.13.1</nodeVersion>
<yarnVersion>v1.22.17</yarnVersion>
...
<goal>yarn</goal> <arguments>install</arguments>
<goal>yarn</goal> <arguments>build</arguments>
```
After this PR that invocation runs **Node 16 + Yarn 1.22** against a project
that:
- no longer has a `yarn.lock` (removed here),
- declares `"engines": { "node": ">=22" }` and ships `.nvmrc` = `22`,
- builds via `webpack -c ./.config/webpack/webpack.config.ts`, which needs
`ts-node` / `@swc/core` from the new toolchain.
So `mvn -Pwith-grafana-plugin package` — which `CLAUDE.md` documents as the
canonical entrypoint for this plugin, with an explicit "do not invoke npm
directly unless you know what the Maven wrapper is doing" — breaks on master
right after this merges.
Roughly what it needs:
```xml
<goal>install-node-and-npm</goal>
<nodeVersion>v22.x.x</nodeVersion> <!-- keep in sync with .nvmrc -->
...
<goal>npm</goal> <arguments>ci</arguments>
<goal>npm</goal> <arguments>run build</arguments>
```
Worth also dropping the `${basedir}/node_modules` / `${basedir}/node` clean
filesets a second look, and noting that `.npmrc`'s `ignore-scripts=true` (a
good default, but an undocumented behaviour change in this PR — please call it
out in the description) applies to the Maven-driven install too.
## 2. Why CI did not catch this, and a suggestion
All 12 checks are green, which is misleading here: **no CI job builds this
module through Maven at all.**
- `compile-check.yml` runs `-P with-all-connectors -P with-examples`, and
`with-all-connectors` in `connectors/pom.xml` does **not** include
`grafana-plugin` — it lives in the separate `with-grafana` /
`with-grafana-plugin` profiles.
- `Jenkinsfile` runs a bare `mvn clean install`, which also excludes those
profiles.
- The green `build` check is `grafana-plugin.yml`, i.e. the npm path this PR
just rewrote — it is verifying its own new path.
The Maven path is a genuine blind spot, not a one-off oversight, so it will
be missed again the same way. Suggest adding a job (either a step in
`compile-check.yml` or a second job in `grafana-plugin.yml`) that runs:
```bash
mvn clean package -Pwith-grafana-plugin -DskipTests -ntp
```
That is cheap, and it is what turns item 1 from "someone has to remember"
into "CI says so".
## 3. Two decisions that belong on dev@
Neither of these is something I think you should just decide in the PR:
**`grafanaDependency` `>=9.3.0` → `>=12.3.0`.** Your reasoning is sound and
well stated — the `@grafana/*` packages are webpack externals resolved from the
host at runtime, so a 13.1.0-only API does not fail the build, it fails in the
user's Grafana with no bundled fallback, and declaring a floor we can actually
stand behind is more honest than a nominal `>=9.3.0`. But the practical effect
is dropping support for Grafana 9/10/11 users of a released Apache plugin, and
that is a project-level compatibility call. Please raise it on dev@ and let the
PMC settle it; if the answer is "keep a lower floor", your own alternative
(pinning the `@grafana/*` packages down instead) is the path.
**The generated `.config/AGENTS/` directory (4 files, ~316 lines).**
`create-plugin` now emits AI-agent instruction files (`instructions.md`,
`e2e-testing.md`, `skills/*.md`) that tell an agent to fetch grafana.com at
runtime and state "Do not modify anything inside the `.config` folder".
Checking agent-directed instructions into an ASF repository is new ground, and
that last rule sits somewhat awkwardly with ASF expectations about governance
of code in the tree. Not objecting — just flagging that it should be a
conscious PMC decision rather than something that arrives as a side effect of a
toolchain bump. Also worth confirming whether
`connectors/grafana-plugin/LICENSE` / `NOTICE` need a pass given the dependency
set changed wholesale, even though these are devDependencies and
`grafana-plugin/**` is RAT-excluded.
---
Item 1 is the only thing that has to change before this can merge. Items 2
and 3 can be follow-ups if the PMC prefers, though item 2 is small enough that
folding it in here is probably easier than tracking it separately.
On the two remaining in-place array mutations in `FromValue.tsx` /
`SelectValue.tsx`: agree with your call to leave them. A toolchain migration is
the wrong place to change untested UI behaviour, and you flagged them
explicitly rather than quietly — a separate PR with tests is the right shape.
--
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]
