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

feat: add suppress_stderr argument to hooks #3771

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

paololazzari
Copy link

Description

Fixes #3770.

Adds suppress_stderr argument to hooks.

TODOs

Read the Gruntwork contribution guidelines.

  • Update the docs.
  • Run the relevant tests successfully, including pre-commit checks.
  • Ensure any 3rd party code adheres with our license policy or delete this line if its not applicable.
  • Include release notes. If this PR is backward incompatible, include a migration guide.

Release Notes (draft)

Added / Removed / Updated [X].

Migration Guide

@paololazzari
Copy link
Author

Given hook.sh:

#!/bin/bash

>&1 echo "stdout"
>&2 echo "stderr"

then with the following hook:

terraform {
  after_hook "after_hook" {
    commands     = ["plan"]
    execute      = ["./hook.sh"]
    run_on_error = true
    suppress_stdout = true
    suppress_stderr = false
  }
}
$ go run main.go plan
...
12:21:35.082 INFO   Executing hook: after_hook
stderr

and with the following hook:

terraform {
  after_hook "after_hook" {
    commands     = ["plan"]
    execute      = ["./hook.sh"]
    run_on_error = true
    suppress_stdout = true
    suppress_stderr = true
  }
}
$ go run main.go plan
...
12:22:05.338 INFO   Executing hook: after_hook

@yhakbar
Copy link
Collaborator

yhakbar commented Jan 16, 2025

Hey @paololazzari ,

Thanks for contributing this. Awesome to see you get engaged in the project!

If you don't mind, could you also add tests to confirm that stderr suppression does and doesn't happen when controlled by the new attribute?

@paololazzari
Copy link
Author

@yhakbar I added a couple of tests. I had to use a shell script to write to stderr. What do you think?

@yhakbar
Copy link
Collaborator

yhakbar commented Jan 17, 2025

@paololazzari , running the tests now. Thanks for adding those! The more recent docs update did result in a conflict with main, though.

Just FYI, I think Feature flags are a good way of creating fixtures that can be changed dynamically. In this scenario, I would have just created one fixture, then toggled stderr suppression so that there's one test with two test cases.

You don't have to change your test, just wanted to make sure you knew that was available for you.

@paololazzari
Copy link
Author

@yhakbar Thanks, I'll look into that. I also updated a couple of existing tests, hopefully I did the right thing there. I'll fix the docs conflict.

@paololazzari
Copy link
Author

@yhakbar just rebased against main

@yhakbar yhakbar self-assigned this Jan 31, 2025
@yhakbar
Copy link
Collaborator

yhakbar commented Jan 31, 2025

Hey @paololazzari ,

We're getting linting errors in CI:

test/integration_hooks_test.go:406:21: S1030: should use showStderr.String() instead of string(showStderr.Bytes()) (gosimple)
        assert.Contains(t, string(showStderr.Bytes()), "ERROR MESSAGE")
                           ^
test/integration_hooks_test.go:424:24: S1030: should use showStderr.String() instead of string(showStderr.Bytes()) (gosimple)
        assert.NotContains(t, string(showStderr.Bytes()), "ERROR MESSAGE")
                              ^

Check out our docs on linting if you need help setting that up locally.

@paololazzari
Copy link
Author

@yhakbar I've addressed those linting errors

@yhakbar yhakbar added the preserved Preserved issues never go stale label Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preserved Preserved issues never go stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add suppress_stderr argument to hooks
2 participants