SEZ9 commented on PR #12858:
URL: https://github.com/apache/seatunnel/pull/12858#issuecomment-6567432129
Thanks for the detailed per-finding breakdown at 5e82f43 — the F1–F3
descriptions line up with what the review asked for:
- **F1** — resolving `seatunnel_home` to an absolute path once (L287) and
passing that absolute script path with `cwd=seatunnel_home` removes the double
resolution. The two tests you list (L460, L348) cover the relative-home and
invalid-home cases, which is what I wanted to see.
- **F2** — classifying lines the way `install-plugin.sh` does, and raising
with the line number before anything is written (L148-153), closes the
trailing-whitespace / trailing-comment gap. The three tests at L192, L206 and
L213 cover the right scenarios.
- **F3** — an `flock` on `config/.plugin_config.lock` via `_exclusive_lock`
(L207-227), held by `install_plugins` (L301) across the whole sequence, is the
right shape.
One thing: your comment appears to have been cut off mid-sentence in F3
("across snapshot, select, ru…"), so I can't see the rest of the F3 explanation
or anything you wrote for F4–F8. Could you repost the missing part?
Specifically I'd like the same per-finding format for:
- **F3 (remainder)** — confirm the lock is released on both the success and
failure paths, and name the test(s) covering it.
- **F4** — how Windows is handled now (use of `bin/install-plugin.cmd`, or
an explicit unsupported-platform error) and what prevents a WSL/Git `bash.exe`
from being handed a Windows path.
- **F5** — what `version` validation rejects before the script starts (`[`,
`,`, `+`, `LATEST`, `-SNAPSHOT`, etc.).
- **F6** — that non-UTF-8 `plugin_config` and `connectors=None` now surface
as `PluginInstallError`.
- **F7** — that the rollback only rewrites the file when `select_plugins`
actually changed it.
- **F8** — the updated example in the error message.
Once I can see those, I'll go through the current head and close out the
remaining items.
<!-- streview-comment:2203 -->
--
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]
