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

Update SyncPluginsForTarget API to allow configuring stdout and stderr externally #88

Merged
merged 1 commit into from
Aug 18, 2023

Conversation

anujc25
Copy link
Contributor

@anujc25 anujc25 commented Aug 15, 2023

What this PR does / why we need it

The TanzuPluginSyncForTarget API is exposed to sync plugins as an experimental API.

  • However, when this API is invoked by a plugin, stdout and stderr are returned as part of the API and are not actively written to the stdout or stderr.
  • Because of this it makes it feel like the CLI is stuck when installing plugins, especially when there large number of plugin that needs to be installed as part of plugin sync API giving subpar UX to the users.

This change exposes additional CommandOptions to configure the outputWriter and errorWriter using WithOutputWriter and WithErrorWriter options when invoking the TanzuPluginSyncForTarget API, along with writing the output directly to the os.Stdout and os.Stderr by default.

Note: Unlike before, the API will now write the output/error of the plugin sync operation to stderr and stdout by default. Callers who previously manage the output by processing the return values of the API call should consider turning off the default writing to the os.Stdout and os.Stderr by passing WithNoStdout and WithNoStderr options as the additional parameters to the function call in order to avoid duplicate echoing.

Which issue(s) this PR fixes

Fixes #

Describe testing done for PR

  • Added unit tests to capture the stdout and stderr along with verifying return types with different combinations

Release note

Fix the `SyncPluginsForTarget` API to allow configuring the `OutputStream` and `ErrorStream` externally. If not configured it defaults the writes to `os.Stdout` and `os.Stderr`

Additional information

Special notes for your reviewer

@anujc25 anujc25 changed the title Update SyncPluginsForTarget API to accept stdout and stderr options Update SyncPluginsForTarget API to configure stdout and stderr externally Aug 15, 2023
@anujc25 anujc25 changed the title Update SyncPluginsForTarget API to configure stdout and stderr externally Update SyncPluginsForTarget API to allow configuring stdout and stderr externally Aug 15, 2023
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really like the flexibility of this approach @anujc25.
So this LGTM if we are ok with breaking the experimental API.

P.S.
If anyone is curious (as I was), after quick googling, I confirmed that this API even allows to write to stdout/stderr and at the same time have the plugin capture the output using io.MultiWriter, to allow a plugin to check the final output.

var out bytes.Buffer
w := io.MultiWriter(os.Stdout, &out)
err := plugin.SyncPluginsForTarget(types.TargetK8s, plugin.WithStderr(w))
// process out.String()

@anujc25
Copy link
Contributor Author

anujc25 commented Aug 16, 2023

Thanks @marckhouzam for the review and feedback. I like the io.MultiWriter approach.

However, I am not sure we should always write to os.Stdout and os.Stderr because it is possible that the user does NOT want to print all logs to os.Stdout or os.Stderr at all and want to overwrite it with something else.

So I am thinking maybe we can use this io.MultiWriter and combine both approaches to give flexibility without changing the API signature.

@anujc25 anujc25 marked this pull request as ready for review August 17, 2023 00:10
@anujc25 anujc25 requested a review from a team as a code owner August 17, 2023 00:10
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice! I think this is a very nice API.

We are mixing terminology between stdout and any other output stream.
For example WithStdout is meant to set a output stream, not necessarily to stdout, while WithNoStdout() is meant to remove the use of stdout. I recommend a bit of renaming. I've put suggestions in-line.

plugin/sync_plugins.go Outdated Show resolved Hide resolved
plugin/sync_plugins.go Outdated Show resolved Hide resolved
plugin/sync_plugins.go Outdated Show resolved Hide resolved
plugin/sync_plugins.go Outdated Show resolved Hide resolved
plugin/sync_plugins.go Outdated Show resolved Hide resolved
plugin/sync_plugins.go Outdated Show resolved Hide resolved
plugin/sync_plugins.go Outdated Show resolved Hide resolved
Copy link
Contributor

@marckhouzam marckhouzam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit about the original example I gave you being wrong but the rest LGTM.
Thanks @anujc25 !

Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is nice enhancement, thanks for the changes. Just some nits on a comment block and the PR description.

On the PR description:

We mention that API outputs to stdout/err by default, but it is not sufficiently clear IMO that this behavior is new.

Maybe a tweak to the description would be to augment with something like
Note: Unlike before, the API will now write the output/error of the plugin sync operation to stderr and stdout by default. Callers who previously manage the output by processing the return values of the API call should consider turning off the default writing to the os.Stdout and os.Stderr by passing WithNoStdout and WithNoStderr options as the additional parameters to the function call in order to avoid duplicate echoing.

It wouldn't hurt to make it a tad clearer in the release note as well.

Finally, given that we mentioned WithNoStdout and WithNoStderr, its seems consistent with describing overriding output and error streams to mention WithOutputWriter and WithErrorWriter as well.

plugin/sync_plugins.go Outdated Show resolved Hide resolved
@anujc25 anujc25 merged commit fb8c932 into vmware-tanzu:main Aug 18, 2023
4 checks passed
@marckhouzam marckhouzam added this to the v1.0.2 milestone Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants