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

Send plugin install output to stderr #7115

Merged
merged 2 commits into from
May 26, 2021
Merged

Send plugin install output to stderr #7115

merged 2 commits into from
May 26, 2021

Conversation

joeduffy
Copy link
Member

We currently send plugin install output to stdout. This interferes
with --json (#5747), automation API scenarios, and in general is bad
CLI hygiene. This change sends plugin output to stdout instead.

Two questions:

  1. Is there a reason this isn't the preferred fix? I can easily believe
    so, since we were exploring alternative ideas in the linked issue.

  2. How should I test this? Where are the current plugin installation
    test cases? I clearly can't add a dependency on AWS here -- is
    there a "simpler" plugin that I can install for test purposes?

CHANGELOG.md Outdated
@@ -1,6 +1,14 @@
CHANGELOG
=========

## Unreleased
Copy link
Contributor

Choose a reason for hiding this comment

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

These need to go into CHANGELOG_PENDING rather than CHANGELOG as we automate the release notes from that file

Copy link
Contributor

@EvanBoyle EvanBoyle left a comment

Choose a reason for hiding this comment

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

The fix looks good to me. I can't think of any reason why we wouldn't do this.

I'm not aware of any e2e tests we have for plugin installation. You could probably pattern match against some of the other e2e CLI tests we have for things like history and install a simple plugin like random:

t.Run("CurrentlySelectedStack", func(t *testing.T) {

Since installing plugins modifies global state, will probably want to set PULUMI_HOME to a tmp dir.

We currently send plugin install output to stdout. This interferes
with --json (#5747), automation API scenarios, and in general is bad
CLI hygiene. This change sends plugin output to stdout instead.
@joeduffy
Copy link
Member Author

You could probably pattern match against some of the other e2e CLI tests we have for things like history and install a simple plugin like random

I thought of that, but doesn't that conceptually add a circular dependency in our repos? That seems...yucky. I had hoped we would have a "no-op" plugin or something we could use for test purposes. I'm kind of surprised we don't have any plugin tests! Since we don't have anything, I may just go as-is, and file an issue to improve our test coverage here. Before giving up and committing, @justinvp I know you've done most work in the plugin system, am I missing an obvious way to test this out?

@joeduffy joeduffy merged commit 7101046 into master May 26, 2021
@pulumi-bot pulumi-bot deleted the joeduffy/issues/5747 branch May 26, 2021 02:02
@justinvp
Copy link
Member

FWIW, we have some unit tests at https://github.com/pulumi/pulumi/tree/master/sdk/go/common/workspace but it doesn't look like we have any E2E tests that call the CLI itself to run a plugin install command. I love the "no-op" plugin idea for testing this case. The plugin doesn't even have to do anything, it could be an empty file. I could see the test spinning up a lightweight http server to serve a no-op tarball and confirming nothing is emitted to standard out while it's being installed.

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

4 participants