Re: [PR] Migrate the Grafana plugin build from grafana-toolkit to create-plugin [iotdb-extras]

["CritasWang (via GitHub)" <gi...@apache.org>]

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: reviews-unsubscribe@iotdb.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org