Allow opt-out of adding stdout/stderr env variables for remote command #451
Conversation
|
I wasn't able to successfully write a test for this one given I couldn't use the preview mode and it was trying to actually SSH into hosts. I'm lacking understanding of how to properly mock this within this plugin. Not sure if the PR can still be merged without the test. |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
Thanks for the PR @julsemaan! The change is simple enough that we could get away without a test. But if you wanted to try your hand at it, I've had success with spinning up a local, in-process SSH server and testing that way. The PR isn't merged yet but you can see it here. |
|
Hey! If I can get away without a test, I'll take it 😄 I could always implement a test in a follow-up (pinky promess?). I'll be able to reuse some of your code vs doing some copy/pasta |
|
Sure, I think we can get away with that :) However, I just noticed that your change is missing adding the new property to |
|
PR is now waiting for a maintainer to run the acceptance tests. |
Good catch, just pushed the change |
Co-authored-by: Thomas Kappler <[email protected]>
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
@julsemaan For any current reviewers and future readers, could you please update the description of your PR to include a detailed explanation of your change and motivation for doing it? Here are some example of good PR descriptions: pulumi/pulumi-kubernetes-operator#575, pulumi/pulumi-github#280, pulumi/pulumi#13066. Thank you for your contribution! |
Hey! That's done, I could suggest to have a PR template so that contributors know more precisely what is expected to provide when opening a PR. Github makes this quite easy, see https://docs.github.com/en/communities/using-templates-to-encourage-useful-issues-and-pull-requests/creating-a-pull-request-template-for-your-repository |
|
@julsemaan, on second thought, I believe we should include test coverage here. If you just copy roughly lines 23-53 from here, that should give you the setup to use SSH in your test. |
|
Hi, @julsemaan! Thank you so much for implementing the fix 😄 Given that this whole thing stems from my feature request, and you never asked to dive into the remote command resource, I went ahead and wrote a test that I think does the job. I sent a pull request to your fork here: julsemaan#1 @thomas11 Do you think the test I wrote looks okay? (If yes, I believe you are also able to pull my commit into this PR, assuming Julien turned maintainer edits on.) |
Add a test for opting remote commands out of PULUMI_COMMAND_STDOUT env var
|
@theneva , thanks for doing the test :) I just pulled it into this branch |
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
@thomas11 Thanks for the review! I tried following your feedback here: https://github.com/julsemaan/pulumi-command/pull/2/files. Since I don't have push access to the main repo or Julien's fork, this workflow kind of sucks for everyone involved… I think it's easiest if you just look at the PR I sent to Julien's branch in case you have more feedback, so that they don't have to jump for every feedback loop. (Sorry for all the notifications, @julsemaan 😅) |
test cleanup
|
PR is now waiting for a maintainer to run the acceptance tests. |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-command/actions/runs/9549446507 |
|
/run-acceptance-tests |
|
Please view the PR build: https://github.com/pulumi/pulumi-command/actions/runs/9549958732 |
|
Merged! Thank you very much for all your efforts, @julsemaan and @theneva! |
Context
At the moment, stdout and stderr of previous runs are passed as environment variables on future invocations. When stdout or stderr is too large, this cause an error.
Proposed change
#355 fixed it for local commands, this fixed it for remote commands. See #285 for full details on the issue
See this comment for details around local vs remote functionality : #285 (comment)