SEZ9 commented on PR #12858: URL: https://github.com/apache/seatunnel/pull/12858#issuecomment-6486012914
Thanks @karthik-0306 for the detailed mapping — that makes the re-review much easier. A few notes before I go through the diff: - **F1–F6**: the approach you describe for each (resolving `seatunnel_home` to an absolute path up front, mirroring the script's first-character rule and failing with a line number before any write, holding `_exclusive_lock` across snapshot → select → run → restore, rejecting Windows before probing for `bash`, validating `version` against the script's character set, and wrapping `UnicodeDecodeError` / `None` / non-string names into `PluginInstallError`) matches what was asked. I'll verify against the code and the listed tests and update the findings accordingly. - **F4**: your reasoning for not dispatching to `bin/install-plugin.cmd` (no `errorlevel` check / no `exit`, so a failed `mvnw.cmd` download would look like success and defeat the rollback) makes sense to me. It would help Windows users if that rationale were visible in the docstring or the error message. - **F7 / F8**: your comment appears cut off partway through the F6 row on my end, so I can't see the rows for the unconditional rollback write when `select_plugins` made no change, or the `'3.0.1'` literal in the error-message hint. Could you repost or point me to where those are handled (and their tests)? Once I have that I'll do a final pass over the diff. <!-- streview-comment:2106 --> -- 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]
