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: adds tagging the TGW attachment + associating / propagating TGW RTB + creating TGW routes #37

Merged
merged 9 commits into from
Feb 20, 2024
Merged

feat: adds tagging the TGW attachment + associating / propagating TGW RTB + creating TGW routes #37

merged 9 commits into from
Feb 20, 2024

Conversation

Gowiem
Copy link
Member

@Gowiem Gowiem commented Feb 20, 2024 •
edited
Loading

major revision

This PR does a good overhaul of the module: It updates to modern practices / syntax + uses new syntax added in TF 1.3 + adds some new, significant functionality + introduces a breaking change (changes var from string => bool type). Considering that, I believe we should release this as 1.0.

what

  • Adds tagging the TGW attachment
  • Adds associating + propagating the TGW attachment if TGW route table given
  • Adds ability to create TGW routes on the given route table
  • Fixes the type of var.vpn_connection_static_routes_only
  • Fixes var.vpc_id being required as this should not be required with TGW VPN Connection
  • Updates to require TF 1.3 and above
  • Removes the null provider
  • Updates vpn_connection_customer_gateway_configuration to sensitive = true
  • Adds transit_gateway_attachment_id as an output

why

  • Adds more flexibility + usefulness when working with a TGW <> VPN connection
  • var.vpn_connection_static_routes_only - Types should be correct, so this was bugging me. Is this possibly a 0.11 holdout that we're finding in 2024? I didn't check the blame, but that'd be funny.
  • Requires 1.3+ TF purely because this should be a 1.0 of this module and I think we should start using new functionality (required here as I used optional in var.transit_gateway_routes)
  • null provider no longer needed - Likely a holdout from an old version of terraform-null-label (this module had some dust on it)

references

@Gowiem
fix: uses correct type with var.vpn_connection_static_routes_only (bo…
9a00c59 
…ol > string)

I could not find why this was originally implemented this way... Maybe a 0.11 holdover? It was bugging me.
@Gowiem
fix: vpc_id should not be required (not required if doing a TGW <> VP…
f89dd6e 
…N connection)
@Gowiem
feat: adds tagging the TGW attachment + associating / propagating TGW…
6849c21 
… RTB + creating TGW routes
@Gowiem
chore: readme updates
586f5e0
@Gowiem Gowiem self-assigned this Feb 20, 2024
@Gowiem Gowiem requested review from a team as code owners February 20, 2024 01:50
@Gowiem Gowiem added enhancement New feature or request major Breaking changes (or first stable release) labels Feb 20, 2024
github-actions[bot]
github-actions bot reviewed Feb 20, 2024
View reviewed changes
outputs.tf Outdated Show resolved Hide resolved
variables.tf Show resolved Hide resolved
@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

Submitting this as a major revision

This PR does a good overhaul of the module: It updates to modern practices / syntax + uses new syntax added in TF 1.3 + adds some new, significant functionality + introduces a breaking change (changes var from string => bool type). Considering that, I believe we should release this as 1.0.

@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem Gowiem enabled auto-merge (squash) February 20, 2024 17:28
@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

1 similar comment
@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem
Copy link
Member Author

Gowiem commented Feb 20, 2024

/terratest

@Gowiem Gowiem requested a review from aknysh February 20, 2024 18:33
aknysh
aknysh approved these changes Feb 20, 2024
View reviewed changes
Copy link
Member

@aknysh aknysh 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 @Gowiem

@Gowiem Gowiem merged commit 576b717 into main Feb 20, 2024
10 checks passed
@Gowiem Gowiem deleted the feature/tgw-association-and-routes branch February 20, 2024 20:12
esolitos pushed a commit to nymedia/terraform-aws-cloudposse-vpn-connection that referenced this pull request Mar 4, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@aknysh aknysh aknysh approved these changes

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

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

Assignees

@Gowiem Gowiem

Labels
enhancement New feature or request major Breaking changes (or first stable release)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Error on plan execution Output Request
2 participants
@Gowiem @aknysh