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

fix logging configuration when enabled and disabled #32

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

Conversation

gpetras
Copy link

@gpetras gpetras commented Jul 7, 2023

This fixes issue #31

what

The Firewall Manager logging configuration would not enable whether or not I supplied my own Kinesis Firehose or not. I tried to turn on firehose_enabled, and I also tried to supply a Firehose ARN using firehose_arn, which I understand conflict with each other when used at the same time. This issue is fixed

I also simplified the logic for generating the logging configuration, as I found it difficult to troubleshoot the multiple nested ternaries.

why

Fixes the logging configuration so that it properly logs to Kinesis Firehose w/ S3

references

closes #31

@gpetras gpetras requested review from a team as code owners July 7, 2023 21:24
@hans-d hans-d added wip Work in Progress: Not ready for final review or merge and removed wip Work in Progress: Not ready for final review or merge labels Mar 2, 2024
@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@hans-d
Copy link

hans-d commented Mar 3, 2024

@gpetras Hi, can you update the pr so that it passes the tests? otherwise, it is likely to be closed due to staleness.

@hans-d hans-d added the stale This PR has gone stale label Mar 3, 2024
Copy link

mergify bot commented Mar 9, 2024

Thanks @gpetras for creating this pull request!

A maintainer will review your changes shortly. Please don't be discouraged if it takes a while.

While you wait, make sure to review our contributor guidelines.

Tip

Need help or want to ask for a PR review to be expedited?

Join us on Slack in the #pr-reviews channel.

@hans-d hans-d added wip Work in Progress: Not ready for final review or merge and removed wip Work in Progress: Not ready for final review or merge labels Mar 10, 2024
@mergify mergify bot added the needs-test Needs testing label Mar 10, 2024
@hans-d
Copy link

hans-d commented Mar 10, 2024

/terratest

@mergify mergify bot removed the stale This PR has gone stale label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-test Needs testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants