Propagate env to process plugin and support command args#2069
Closed
anuraaga wants to merge 1 commit intosqlc-dev:mainfrom
Closed
Propagate env to process plugin and support command args#2069anuraaga wants to merge 1 commit intosqlc-dev:mainfrom
anuraaga wants to merge 1 commit intosqlc-dev:mainfrom
Conversation
Collaborator
|
@anuraaga Thank you so much for the pull request. I'm sorry that it's taken a few months for me to respond. As it is, I'm not comfortable merging this for a few reasons. Resetting the environment variables was a specific choice: this makes it a bit more difficult for process plugins to silently exfiltrate environment variables. As for the commands themselves, I'd like to continue following the protoc model for plugins when don't allow for custom arguments. If you have more specific issues with the current setup, happy to hear what those are so that we can figure out a way forward. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is a strawman's proposal that I noticed while working on #2068 and feel free to reject it. Unit tests wouldn't pass because they expect installation of
sqlc-gen-jsonon the running machine. I guess it's best if tests can be run without any external dependencies outside the codebase. Luckily,go runis basically guaranteed to work fine and could be an appropriate alternative.I started by adding support for splitting args out of the cmd string. Then I also noticed that the user's environment is not propagated to the process when run - I think generally users would expect the environment to be available when an external process is run as the plugin may have dependencies on it, and it would otherwise be impossible to run the plugin. Though if not propagating was intentional, then that's fine too and definitely this PR can be closed.