Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

For plugins, only enable color output if command is running in a terminal #1015

Merged
merged 5 commits into from
Jan 10, 2023

Conversation

vcheung-stripe
Copy link
Collaborator

@vcheung-stripe vcheung-stripe commented Jan 4, 2023

Reviewers

r? @
cc @stripe/developer-products

Summary

This PR changes the plugin interface so that the host tells the plugin process if the host's stdin/out/err are terminals or not. Plugins can read this this to decide if they should print color output. In addition, this opens up the possibility of adding other terminal-only features, evolving the plugin interface, and separating concerns between host and plugin.

This change is needed because functions like term.IsTerminal always return false when called inside the plugin due to how the plugin's file descriptors are managed by the host process.

To change the interface, we leverage the go-plugin package's versioning ability to implement a "major change" to the plugin interface. The old interface is defined as:

type Dispatcher interface {
	RunCommand(args []string) (string, error)
}

while the new interface is defined as:

type DispatcherGRPC interface {
	RunCommand(additionalInfo *proto.AdditionalInfo, args []string) error
}

This new interface uses gRPC instead of net/rpc to handle IPC between the host and plugin. gRPC is needed because the net/rpc implementation can't handle serializing complex data structures, which we've introduced in a struct called AdditionalInfo.

This change is backwards compatible because the go-plugin library automatically handles version negotiation. Although the new interface is backwards incompatible, we keep the old interface around so older clients can continue to request the old interface.

@vcheung-stripe vcheung-stripe changed the title Enable color output only if plugin command is running in a terminal For plugins, only enable color output if command is running in a terminal Jan 4, 2023
@vcheung-stripe vcheung-stripe marked this pull request as ready for review January 9, 2023 19:52
@vcheung-stripe vcheung-stripe requested a review from a team as a code owner January 9, 2023 19:52
@vcheung-stripe vcheung-stripe merged commit 0238a84 into master Jan 10, 2023
@vcheung-stripe vcheung-stripe deleted the vcheung/fix-plugins-colors branch January 10, 2023 19:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants