SEZ9 commented on issue #12707: URL: https://github.com/apache/seatunnel/issues/12707#issuecomment-6258354761
@karthik-0306 thanks for getting #12858 up so quickly after #12857 — this is exactly the small, self-contained slice I was hoping for. On the design choice: keeping non-requested connectors commented with a leading `#` rather than deleting them is fine with me. Since `bin/install-plugin.sh` already skips `#` lines, the installed result is identical, and preserving the full list with its section headers means a later call can re-enable a different connector without us having to reconstruct the file. No need to switch to deleting. Two asks before I review #12858 in detail: 1. Make the ownership boundary explicit in the module docstring and PR description: the helper must only rewrite the `config/plugin_config` inside a launcher-managed copy (per-user / per-version cache or whatever the caller hands it), never a source checkout or a shared installation. I'd like that to be obvious to anyone who imports the function later for the local-run path. 2. Keep the idempotence case (running twice gives the same file) and the re-enable case (connector A, then connector B, with A ending up commented and B active) as explicit regression tests, alongside the mocked `install-plugin.sh` invocation. Once those are in I'll go through the PR. Same pattern as #12740 worked well, so if @CryoThrust has a chance to exercise the helper against a real distribution on macOS as before, that would be a useful extra data point, but it's not a blocker. <!-- streview-comment:1856 --> -- 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]
