karthik-0306 commented on PR #12858:
URL: https://github.com/apache/seatunnel/pull/12858#issuecomment-6410015392
Thanks a lot @SEZ9 for the thorough review. All 8 points are addressed in
the latest commit:
1. **Relative `seatunnel_home`:** it's now resolved with
`os.path.abspath(os.path.expanduser(...))` before building any path, so bash
always gets an absolute script path. Test:
`test_relative_home_is_resolved_to_an_absolute_script_path`. I also ran it on
Linux against the real `install-plugin.sh` with a relative home, and
`connector-assert-3.0.0.jar` was installed.
2. **Line parsing:** lines are now classified exactly like
`install-plugin.sh` (first character: blank / `-` / `#` skipped, anything else
is a connector). Whitespace around a connector ID is tolerated
(`#connector-kafka ` is recognized as disabled), and any line the script would
reject (`connector-kafka # keep`, whitespace-only, an indented comment) raises
`PluginInstallError` with its line number before anything is rewritten.
3. **Concurrency:** `install_plugins()` now holds an exclusive `fcntl.flock`
on `config/.plugin_config.lock` across snapshot → select → run → restore.
`test_lock_is_held_while_the_script_runs` checks that another process can't
take the lock during the run (runs on Linux/macOS). I also ran two real
processes against one copy on Linux: they were serialized, and each script saw
its own selection.
4. **Windows:** I went with your second option. `install_plugins()` now
raises a clear "Linux and macOS only" error before touching `plugin_config` or
looking for bash, and the docstrings say so. I didn't dispatch to
`install-plugin.cmd` because it never checks `errorlevel` or exits non-zero, so
a failed `mvnw.cmd` download would look like success and the rollback couldn't
work. `select_plugins()` itself stays cross-platform.
5. **Version:** validated with the script's character set before any
rewrite, and the docstring now mentions the Maven fallback for `LATEST` /
`RELEASE` / `*-SNAPSHOT`.
6. **Error contract:** `UnicodeDecodeError` is wrapped in
`PluginInstallError`, and `connectors=None` / non-string names raise
`PluginInstallError` showing the original value.
7. **Restore only when changed:** the rollback now compares the current
bytes with the snapshot and only writes if they differ
(`test_failed_install_does_not_rewrite_an_unchanged_file`).
8. **Version hint:** no literal release in the message anymore ("it should
match your SeaTunnel engine version").
10 new tests (37 in this module). All 65 SDK tests pass on Windows (Python
3.9 / 3.11 / 3.13; the lock test is skipped there, since there's no `fcntl`)
and on Linux (Python 3.10, with the lock test running).
--
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]
