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: support for 32-bit ASN #52

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

Conversation

riccardolocci
Copy link
Contributor

The variable customer_gateway_bgp_asn now automatically sets bgp_asn or bgp_asn_extended whether the value cannot be represented as a 32-bit number

what

This PR adds support for ASN numbers greater than 2147483647

why

Customer Gateways created by the module only use variable bgp_asn, which does not support 32-bit integers
Instead the aws_customer_gateway resource provides a different variable, bgp_asn_extended which is not currently used by the module

references

hashicorp/terraform-provider-aws#38738

The variable `customer_gateway_bgp_asn` now automatically sets `bgp_asn` or `bgp_asn_extended` whether the value cannot be represented as a 32-bit number
@riccardolocci riccardolocci requested review from a team as code owners October 9, 2024 14:37
@mergify mergify bot added the triage Needs triage label Oct 9, 2024
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.

@riccardolocci Looks like a fine change, but you'll need to run terraform format and push those changes, otherwise our tests will fail. Do that and I'll re-review.

@riccardolocci
Copy link
Contributor Author

@Gowiem thank you for the comment, I've applied the changes you requested and aligned the PR with the latest commits.

@Gowiem Gowiem added patch A minor, backward compatible change bugfix Change that restores intended behavior labels Oct 12, 2024
@Gowiem Gowiem changed the title Added support for 32-bit ASN fix: support for 32-bit ASN Oct 12, 2024
@Gowiem
Copy link
Member

Gowiem commented Oct 12, 2024

/terratest

Gowiem
Gowiem previously approved these changes Oct 12, 2024
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 good. One request to check into, but otherwise let's ship it :shipit:

main.tf Show resolved Hide resolved
@mergify mergify bot removed the triage Needs triage label Oct 12, 2024
@riccardolocci
Copy link
Contributor Author

Hi @Gowiem, I've updated the versions.tf with the correct pin.

Copy link

mergify bot commented Oct 16, 2024

💥 This pull request now has conflicts. Could you fix it @riccardolocci? 🙏

@mergify mergify bot added the conflict This PR has conflicts label Oct 16, 2024
@mergify mergify bot removed the conflict This PR has conflicts label Oct 16, 2024
@riccardolocci
Copy link
Contributor Author

Merged latest changes from main and updated documentation due to AWS provider version update

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Change that restores intended behavior patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants