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

Log output while Popen is in progress #45

Merged
merged 8 commits into from
May 4, 2022

Conversation

leighpascoe
Copy link
Contributor

Currently, the output is logged after the process has completed, this could be a problem for longer running terraform/terragrunt actions. It may take over 10 minutes to create a resource, or be locked on deleting a resources. The user would not know what's happening until the completion of the process.

I propose polling and logging the output during the process's runtime. this will provide valuable feedback to the user.

leighpascoe and others added 6 commits April 22, 2022 15:17
Currently the `execute_command` function will only log the output after the command has completed, considering that the resources being created may take a long time, getting feedback during execution would be very valuable.
Copy link
Collaborator

@ludoo ludoo left a comment

Choose a reason for hiding this comment

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

Hey, thanks for this it makes complete sense.

I dropped a couple minor comments. Can you also lint with autopep8 using 2 spaces [1], and add a simple test? [2]

[1] use a ~/.config/pep8 file with these two lines

[pep8]
indent-size = 2

[2] Something like this in test_plan.py

def test_plan_stdout(fixtures_dir):
  tf = tftest.TerraformTest('plan_no_resource_changes', fixtures_dir)
  result = tf.plan(output=False)
  assert 'just_an_output = "Hello, plan!"' in result

tftest.py Outdated Show resolved Hide resolved
tftest.py Show resolved Hide resolved
tftest.py Outdated Show resolved Hide resolved
@ludoo
Copy link
Collaborator

ludoo commented May 4, 2022

I switched to using GitHub workflows, you should be able to check lint and test errors here in the PR. Thanks again for submitting this!

@ludoo ludoo added the enhancement New feature or request label May 4, 2022
- fixed formatting
- used list instead of string concat
- add encoding and ignore error params to popen
@ludoo
Copy link
Collaborator

ludoo commented May 4, 2022

Thanks for the changes, you should be able to merge now. Let me know if anything is preventing you from merging!

@leighpascoe
Copy link
Contributor Author

Merging is blocked. It says The base branch restricts merging to authorized users.

@ludoo
Copy link
Collaborator

ludoo commented May 4, 2022

Merging is blocked. It says The base branch restricts merging to authorized users.

Hm, as a first time contributor I think you're prevented from running checks and merging. Guess you'll have to seend another PR soon to verify that. :)

Let me merge for you!

@ludoo ludoo merged commit 414be8b into GoogleCloudPlatform:master May 4, 2022
@leighpascoe
Copy link
Contributor Author

Thank you for your help and providing this tool. We're using this library extensively for our terraform module testing framework. I am hoping to make improvements in the future when i can.

@leighpascoe leighpascoe deleted the patch-1 branch May 4, 2022 13:49
@ludoo
Copy link
Collaborator

ludoo commented May 4, 2022

Thank you for your help and providing this tool. We're using this library extensively for our terraform module testing framework. I am hoping to make improvements in the future when i can.

Thank you for this PR, and sorry if I lagged a bit!

@leighpascoe
Copy link
Contributor Author

When do you expect to release next? We are looking forward to this enhancement

@ludoo
Copy link
Collaborator

ludoo commented May 4, 2022

Good point, let me try doing it in the next few minutes. :)

@ludoo
Copy link
Collaborator

ludoo commented May 4, 2022

https://pypi.org/project/tftest/1.6.5/

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants