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

Consider Adding Channel Direction in tf5server/tf6server WithDebug() Function Signature #144

Closed
bflad opened this issue Jan 25, 2022 · 2 comments
Labels
enhancement New feature or request

Comments

@bflad
Copy link
Contributor

bflad commented Jan 25, 2022

terraform-plugin-go version

v0.7.0

Use cases

Downstream SDK/server implementations may already have an existing go-plugin ServeTestConfig, whose fields include channel directionality in the type:

type ServeTestConfig struct {
  // ...
  ReattachConfigCh chan<- *ReattachConfig
  CloseCh chan<- struct{}
  // ...
}

The current tf5server and tf6server implementations have a function signature that does not include the channel directionality:

func WithDebug(ctx context.Context, config chan *plugin.ReattachConfig, closeCh chan struct{}) ServeOpt { /* */ }

Since the function signature states it wants a bi-directional channel, it is unnecessarily difficult to pass the channels from the go-plugin fields through the function, which winds up in the same type anyways.

Attempted solutions

Workaround:

closeCh := make(chan struct{})
reattachConfigCh := make(chan *plugin.ReattachConfig)

go func() {
	val, ok := <-closeCh

	if ok {
		opts.TestConfig.CloseCh <- val
	}
}()

go func() {
	val, ok := <-reattachConfigCh

	if ok {
		opts.TestConfig.ReattachConfigCh <- val
	}
}()

tf5serveOpts = append(tf5serveOpts, tf5server.WithDebug(
	opts.TestConfig.Context,
	reattachConfigCh,
	closeCh),
)

Proposal

Add <- to function signature.

func WithDebug(ctx context.Context, config chan<- *plugin.ReattachConfig, closeCh chan<- struct{}) ServeOpt { /* */ }

Then the implementation becomes passthrough:

tf5serveOpts = append(tf5serveOpts, tf5server.WithDebug(
	opts.TestConfig.Context,
	opts.TestConfig.ReattachConfigCh,
	opts.TestConfig.CloseCh),
)

References

@bflad bflad added the enhancement New feature or request label Jan 25, 2022
@bflad
Copy link
Contributor Author

bflad commented Jul 1, 2022

Closing as this isn't easy given the current code. I'm sure it could be simplified somehow, but its not a near-term priority.

@bflad bflad closed this as completed Jul 1, 2022
Copy link

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 23, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant