karthik-0306 commented on PR #12858: URL: https://github.com/apache/seatunnel/pull/12858#issuecomment-6477191772
Thanks @SEZ9, and thanks @DanielLeens for the summary. I've rebased onto the latest `dev`, which includes #12994 (SDK workflow pinned to `ubuntu-24.04`), so `Test with Python 3.9` can run now. No code changed in the rebase. Here is each finding mapped to the code (line numbers in `tools/seatunnel-python-sdk/seatunnel/plugin_installer.py`) and its test in `tests/test_plugin_installer.py`. Nothing is deferred: | Finding | Where | Test(s) | |---|---|---| | **F1** relative `seatunnel_home` | `:286` resolves `os.path.abspath(os.path.expanduser(...))` before any path is built; `:279-284` rejects a non-path home | `test_relative_home_is_resolved_to_an_absolute_script_path`, `test_invalid_seatunnel_home_raises_plugin_install_error` | | **F2** line parsing | `:133-153`: same first-character rule as `install-plugin.sh` (blank / `-` / `#` skipped, anything else is a connector); whitespace tolerated (`:138`), a line the script would reject raises with its line number before any write (`:148-153`) | `test_connector_lines_with_stray_whitespace_are_recognized`, `test_disabled_connector_with_trailing_whitespace_can_be_enabled`, `test_line_rejected_by_install_script_raises_and_leaves_file_unchanged` | | **F3** inter-process lock | `_exclusive_lock` (`:207-227`) takes `fcntl.flock(LOCK_EX)` on `config/.plugin_config.lock`; `install_plugins` holds it for the whole snapshot → select → run → restore (`:300`) | `test_lock_is_held_while_the_script_runs` (another process cannot take the lock during the run; runs on Linux/macOS, including the Ubuntu CI job) | | **F4** Windows | `install_plugins` rejects Windows first (`:262`), before looking for bash or touching `plugin_config`, so the POSIX-only lock is never reached there. Not dispatched to `bin/install-plugin.cmd`: it has no `errorlevel` check and no `exit`, so a failed `mvnw.cmd` download returns success and the rollback could not trigger. `select_plugins()` stays cross-platform | `test_windows_is_rejected_before_rewriting` | | **F5** version | `:273` validates with the script's character set before any rewrite; Maven fallback for `LATEST` / `RELEASE` / `*-SNAPSHOT` documented in the docstring | `test_invalid_version_raises_before_rewriting`, `test_empty_version_raises` | | **F6** error contract | `UnicodeDecodeError` wrapped (`:120`); `None` / non-iterable (`:71`) and non-string names (`:77`) raise `PluginInstallError` with the original value | `test_non_utf8_file_raises_plugin_install_error`, `test_invalid_connectors_argument_raises_plugin_install_error` | | **F7** restore only when changed | `_restore_if_changed` (`:191-199`) writes only if the current bytes differ from the snapshot; used at `:327` | `test_failed_install_does_not_rewrite_an_unchanged_file` | | **F8** version hint | no literal release in the messages anymore (`:271`, `:276`: "it should match your SeaTunnel engine version") | `test_empty_version_raises` (asserts the message has no `3.0.1`) | On concurrency, beyond the unit test: I also ran two real `install_plugins` processes against one copy on Linux. They were serialized, and each `install-plugin.sh` run saw only its own selection. All 67 SDK tests pass on Linux with Python 3.9, 3.10 and 3.13 (lock test included). -- 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]
