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(connectivity): custom Settings for Virtual Hub connection names #885

Merged
merged 4 commits into from
Feb 10, 2024

Conversation

birdnathan
Copy link
Contributor

Overview/Summary

Been an outstanding request for a while but finally got a chance to look at this properly. The additional capability will allow users to override a Virtual Hub Virtual Network Connection name. The logic is the same as what was added for hub/spoke peerings via #401

This PR fixes/adds/changes/removes

  1. Naming of the virtual hub connection #633

Testing Evidence

Evidence from local run:

Virtual Network Resource IDs added to relevant arrays withing settings.connectivity.tf:
image
Without a custom setting we get a randomly generated connection name:
image
With the custom setting for the target virtual network resource id added to the settings.connectivity.tf:
image
The random name is overridden:
image
Portal Views following deployment:
image
image

Adding a custom setting to a currently connected virtual network or changing the name will destroy and recreate the connection. Terraform waits for the destroy to complete before re-adding the connection.
image

As part of this Pull Request I have

  • Checked for duplicate Pull Requests
  • Associated it with relevant issues, for tracking and closure.
  • Ensured my code/branch is up-to-date with the latest changes in the main branch
  • Performed testing and provided evidence.
  • Updated relevant and associated documentation.

@birdnathan
Copy link
Contributor Author

birdnathan commented Jan 5, 2024 via email

@birdnathan
Copy link
Contributor Author

@matt-FFFFFF - Can we get this reviewed at all? About to deploy a large vwan architecture for a customer and be nice to have this available to use. TIA

@matt-FFFFFF
Copy link
Member

Hi @birdnathan

Sorry this slipped through the net

Thank you so much for the work here. I will review properly next week

@matt-FFFFFF matt-FFFFFF self-assigned this Feb 4, 2024
@matt-FFFFFF matt-FFFFFF changed the title Feature Request - Custom Settings for Virtual Hub connection names feat(connectivity): custom Settings for Virtual Hub connection names Feb 4, 2024
@matt-FFFFFF
Copy link
Member

@birdnathan looks like the linter isn't happy 🙂

Warning: local.virtual_hub_connection_resource_id is declared but not used (terraform_unused_declarations)

Did you mean to use this local anywhere?

@birdnathan
Copy link
Contributor Author

@matt-FFFFFF - Fix committed.

@matt-FFFFFF
Copy link
Member

/azp run unit

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matt-FFFFFF
Copy link
Member

Hi @birdnathan

The tests are giving this error on plan - the test in question is in the tests/modules/test_001_baseline dir if you want to try it yourself.

╷
│ Error: Invalid index
│ 
│   on ../../../modules/connectivity/locals.tf line 1847, in locals:
│ 1847:           resource_id       = local.virtual_hub_connection_resource_id[location][spoke_resource_id]
│     ├────────────────
│     │ local.virtual_hub_connection_resource_id is object with 3 attributes
│ 
│ The given key does not identify an element in this collection value.
╵
╷
│ Error: Invalid index
│ 
│   on ../../../modules/connectivity/locals.tf line 1850, in locals:
│ 1850:           name                      = local.virtual_hub_connection_name[location][spoke_resource_id]
│     ├────────────────
│     │ local.virtual_hub_connection_name is object with 3 attributes
│ 
│ The given key does not identify an element in this collection value.
╵
make: *** [Makefile:41: tf-plan] Error 1

@birdnathan
Copy link
Contributor Author

How's that now @matt-FFFFFF ? The more I looked at it the simpler I could actually make it 🙃

Copy link

Commenter does not have sufficient privileges for PR 885 in repo Azure/terraform-azurerm-caf-enterprise-scale

@matt-FFFFFF
Copy link
Member

/azp run unit

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@matt-FFFFFF
Copy link
Member

/azp run e2e

Copy link

No pipelines are associated with this pull request.

@matt-FFFFFF
Copy link
Member

Looks good @birdnathan

Thanks for your efforts here. Just running final test and will merge

@matt-FFFFFF matt-FFFFFF merged commit 4bb8288 into Azure:main Feb 10, 2024
9 checks passed
@matt-FFFFFF
Copy link
Member

released in v5.1.0 - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants