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.

Sign up for GitHub

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

Jump to bottom

feat: add logging options #26

Merged
merged 12 commits into from Mar 9, 2024
Merged

feat: add logging options #26

merged 12 commits into from Mar 9, 2024

Conversation

esolitos
Copy link
Contributor

@esolitos esolitos commented Mar 13, 2023 •
edited

what

Implemented ability to set log to cloudwatch.

  • Added cloudposse/cloudwatch-logs/aws to create a logging group
  • Added vpn_connection_tunnel1_cloudwatch_log_enabled and vpn_connection_tunnel2_cloudwatch_log_enabled to allow enabling logging for a specific tunnel
  • Added vpn_connection_log_retention_in_days to allow configuring logs retention
  • Ran make actions to sync, as recommended in another PR.

why

This allow enabling logging on the tunnels, which was not possible before.

references

@esolitos esolitos requested review from a team as code owners March 13, 2023 13:31
Gowiem
Gowiem requested changes Feb 19, 2024
View reviewed changes
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

@esolitos this is looking solid, couple very small things and we can get this merged. Check it out and ping me when you're ready! Thanks!

main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Show resolved Hide resolved
variables.tf Outdated Show resolved Hide resolved
@Gowiem Gowiem self-requested a review February 19, 2024 22:08
@Gowiem Gowiem self-assigned this Feb 19, 2024
@esolitos @Gowiem
Apply suggestions from code review by @Gowiem
1095ec2 
Co-authored-by: Matt Gowie <matt@masterpoint.io>
@esolitos
Copy link
Contributor Author

esolitos commented Feb 20, 2024 •
edited

I just added the suggestions for now. Will try to update the rest when I have some extra minutes. :)

Edit: And of course.. Thank you for the review. 👍🏼 👍🏼

@Gowiem Gowiem added enhancement New feature or request minor New features that do not break anything labels Feb 20, 2024
@hans-d
Copy link

hans-d commented Mar 2, 2024

/terratest

@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 3, 2024
@Gowiem
Copy link
Member

Gowiem commented Mar 4, 2024

@esolitos friendly ping on the above -- Would love to get this in as it looks like a solid implementation. Please give it another pass when you get the chance!

@esolitos
chore: force document updates as in main
baa7b6d
@esolitos
Merge remote-tracking branch 'upstream/main' into feat/logging
1dc6907 
* upstream/main:
  feat: adds tagging the TGW attachment + associating / propagating TGW RTB + creating TGW routes (cloudposse#37)
  Update README.md and docs (cloudposse#31)
  Sync github (cloudposse#29)
  Feature: Multiple VPCs through single site-to-site VPN with transit gateway id (cloudposse#27)
@esolitos
chore: make readme
17ccaac
@esolitos
chore: make github/init
996a37c
@Gowiem
Copy link
Member

Gowiem commented Mar 4, 2024

/terratest

@esolitos
Copy link
Contributor Author

esolitos commented Mar 4, 2024

Thanks for the reminder. :)

Gowiem
Gowiem reviewed Mar 4, 2024
View reviewed changes
variables.tf Outdated Show resolved Hide resolved
Gowiem
Gowiem requested changes Mar 4, 2024
View reviewed changes
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

Looks really good, one change suggested

@esolitos @Gowiem
fix: missed in merge
e3f68b3 
Co-authored-by: Matt Gowie <matt@masterpoint.io>
@Gowiem
Copy link
Member

Gowiem commented Mar 4, 2024

/terratest

Gowiem
Gowiem approved these changes Mar 4, 2024
View reviewed changes
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

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

LGTM -- Thanks for working through @esolitos!

@Gowiem Gowiem enabled auto-merge (squash) March 4, 2024 19:46
@Gowiem Gowiem added the needs-cp label Mar 4, 2024
@mergify Mergify
Copy link

mergify bot commented Mar 9, 2024

Thanks @esolitos 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.

@mergify Mergify
Copy link

mergify bot commented Mar 9, 2024

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

@mergify mergify bot added the needs-cloudposse Needs Cloud Posse assistance label Mar 9, 2024
@Gowiem Gowiem merged commit 1681fa0 into cloudposse:main Mar 9, 2024
11 checks passed
@mergify mergify bot removed the needs-cloudposse Needs Cloud Posse assistance label Mar 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gowiem Gowiem Gowiem approved these changes

@korenyoni korenyoni Awaiting requested review from korenyoni korenyoni is a code owner automatically assigned from cloudposse/contributors

Assignees

@Gowiem Gowiem

Labels
enhancement New feature or request minor New features that do not break anything
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add logs configuration
3 participants
@esolitos @hans-d @Gowiem