Skip to content
This repository has been archived by the owner on Mar 8, 2021. It is now read-only.

Support stdin and stdout from piped process #33

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

pecigonzalo
Copy link

@pecigonzalo pecigonzalo commented May 15, 2019

Fixes: #32

Only format the content between ------ and output the rest, additionally allowing the user to pass input more clearly.

@dmlittle This a WIP implementation of the changes I requested on #32, I want to clean the code + add tests, but first wanted to check with you, is in scope of this to also format apply or only plan? because apply does not output ---.

Manual tests

Subproc and input

Test that a subproc and easily show and receive input. Previously, the input was passed, but because we could not see the output, it was hard to tell if we had to pass input, basically it was a bad unix pipe citizen.

./test/subproc.sh | ./scenery

New Output:

❯ ./test/subproc.sh | ./scenery
Enter some text:
asd
you typed: asd

Previous Output:

❯ ./test/subproc.sh | scenery
Format only between ----

Format only the code between ----, output the rest to the console, to make it easier to interact and see progress of commands, wrappers and even the initial part of the terraform plan.

pushd test
terraform init
terraform apply
# Modify test/main.tf actions
popd


./test/wrapper.sh | ./scenery

New Output:

❯ ./test/wrapper.sh | ./scenery
+ pushd test
+ trap popd EXIT
~/Workspace/src/scenery/test ~/Workspace/src/scenery
+ echo asdasd
asdasd
+ echo '!23123'
!23123
+ terraform plan -input=true -lock=false
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

data.aws_iam_policy_document.write: Refreshing state...
aws_iam_policy.write: Refreshing state... (ID: arn:aws:iam::155161382138:policy/testpolicy-deleteme)

------------------------------------------------------------------------
~ aws_iam_policy.write
    policy:  {
               "Statement": [
                 {
                   "Action": [
            -        "ecr:UploadLayerPart",
            +        "ecr:UploadLayer*",
                     "ecr:PutImage",
                     "ecr:InitiateLayerUpload",
                     "ecr:CompleteLayerUpload"
                   ],
                   "Effect": "Allow",
            

Plan: 0 to add, 1 to change, 0 to destroy.
------------------------------------------------------------------------

Note: You didn't specify an "-out" parameter to save this plan, so Terraform
can't guarantee that exactly these actions will be performed if
"terraform apply" is subsequently run.

+ popd
~/Workspace/src/scenery

Previous Output:

❯ ./test/wrapper.sh | scenery  
+ pushd test
+ trap popd EXIT
+ echo asdasd
+ echo '!23123'
+ terraform plan -input=true -lock=false
+ popd
~ aws_iam_policy.write
    policy:  {
               "Statement": [
                 {
                   "Action": [
            -        "ecr:UploadLayerPart",
            +        "ecr:UploadLayer*",
                     "ecr:PutImage",
                     "ecr:InitiateLayerUpload",
                     "ecr:CompleteLayerUpload"
                   ],
                   "Effect": "Allow",
            

Plan: 0 to add, 1 to change, 0 to destroy.

@pecigonzalo pecigonzalo changed the title Support stdin and stdout from piped process WIP: Support stdin and stdout from piped process May 15, 2019
@dmlittle
Copy link
Owner

@pecigonzalo thanks for starting work on this issue!

is in scope of this to also format apply or only plan? because apply does not output ---.
I think there are 2 different issues currently open to add extra functionality:

  • Feature: Show non-plan output #32 deals with adding the ability to show non-terraform plan output (things included before or after the plan). This doesn't need to add the ability to interact with prompts in the terraform commands. While I'm OK with adding this functionality I don't want this to be the default behavior of scenery and it should be enabled only when run with a special flag (maybe --full-output/-F).
  • Add support for Terraform/Terragrunt Apply #30 deals with the ability to interact with the terraform apply command that asks for user-input.

I think we should tackle these 2 issues in different PRs. You do not need to add the ability to parse terraform apply output if you don't want to as this is currently not supported by scenery.

@pecigonzalo
Copy link
Author

Indeed, im not trying to fix 30, only 32. I was just ensuring I was not removing any previous functionality.
I actually believe, it should not parse apply, only plan.

Regarding #32 and the -F option, I actually think it should be the opposite, right now scenery is not a good "unix" citizen, as its destroying the ability to combine or pipe other tools inline. I would prefer to make this default and have an option to remove it, as in its current behavior it prevents even terraform errors or prompts. Let me know your thoughts

@pecigonzalo pecigonzalo changed the title WIP: Support stdin and stdout from piped process Support stdin and stdout from piped process Jun 6, 2019
Only format the content between  and output the rest, additionally allowing the user to pass input more clearly.
@pecigonzalo pecigonzalo force-pushed the feature/handle-stdin-and-stdout branch from bdb35b5 to f9e85ad Compare June 6, 2019 11:06
@pecigonzalo
Copy link
Author

@dmlittle Anything that holds merging this?

@dmlittle
Copy link
Owner

@pecigonzalo sorry for the delayed response, I've been busy these past few weeks and just had time to catch up on this issue. As I mentioned earlier I'm willing to include this functionality as an optional flag but not as the default behavior.

I don't necessarily agree with the Unix good citizen argument. The purpose of this tool is to make a terraform plan output easier to read by stripping away extraneous information and prettifying things when possible. If you have a script that calls terraform plan and the output of that script is being piped into scenery you can fix the stripping of information by scenery by calling scenery within the script instead of piping the entire script into scenery.

As for scenery preventing terraform errors and/or prompts that's a bug and should be fixed. I'll look more into it.

@pecigonzalo
Copy link
Author

pecigonzalo commented Jun 17, 2019

@dmlittle No worries, I wasnt sure how to proceed.

As for scenery preventing terraform errors and/or prompts that's a bug and should be fixed. I'll look more into it.

This is exactly what this change is doing, is covered by it.

AFAIK you cant detect other errors/prompts in a generic enough way to craft a "only these prompts" so you cant do this in a flag. You either pass stdin or not, I can make a flag that fixes the issue or not, but not fix the issue and also optionally strip other content from stdin.

Event in this case:

If you have a script that calls terraform plan and the output of that script is being piped into scenery you can fix the stripping of information by scenery by calling scenery within the script instead of piping the entire script into scenery.

Its not behaving and its stripping stderr/stdin content, preventing prompts and errors from going out.

@pecigonzalo
Copy link
Author

@dmlittle any comments on this?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Show non-plan output
2 participants