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

New Resource azurerm_resource_management_private_link_association #23546

Merged
merged 6 commits into from
Oct 24, 2023

Conversation

teowa
Copy link
Contributor

@teowa teowa commented Oct 13, 2023

Private link association (Microsoft.Authorization/privateLinkAssociations)

TF_ACC=1 go test -v ./internal/services/resource -parallel 20 -run TestAccResourceManagementPrivateLinkAssociation -timeout 2h -ldflags="-X=github.com/hashicorp/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccResourceManagementPrivateLinkAssociation_basic
=== PAUSE TestAccResourceManagementPrivateLinkAssociation_basic
=== RUN   TestAccResourceManagementPrivateLinkAssociation_name
=== PAUSE TestAccResourceManagementPrivateLinkAssociation_name
=== RUN   TestAccResourceManagementPrivateLinkAssociation_requiresImport
=== PAUSE TestAccResourceManagementPrivateLinkAssociation_requiresImport
=== CONT  TestAccResourceManagementPrivateLinkAssociation_basic
=== CONT  TestAccResourceManagementPrivateLinkAssociation_requiresImport
=== CONT  TestAccResourceManagementPrivateLinkAssociation_name
--- PASS: TestAccResourceManagementPrivateLinkAssociation_basic (130.37s)
--- PASS: TestAccResourceManagementPrivateLinkAssociation_requiresImport (131.88s)
--- PASS: TestAccResourceManagementPrivateLinkAssociation_name (135.95s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/resource      135.971s

This resource works together with azurerm_resource_management_private_link #23098
image

Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Thanks @teowa - I've left a couple of comments inline but once those are addressed we can take another look. Thanks!

Comment on lines 45 to 51
"name": {
ForceNew: true,
Optional: true,
Computed: true,
Type: pluginsdk.TypeString,
ValidateFunc: validation.IsUUID,
},
Copy link
Member

Choose a reason for hiding this comment

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

Given that this is only used as part of the resource id, would it make sense to remove this entirely and rely on the generated uuid here? In any case, as outlined in the contributors guide here we are no longer adding Optional and Computed to new fields.

Copy link
Contributor Author

@teowa teowa Oct 17, 2023

Choose a reason for hiding this comment

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

If remove name entirely, will get test error:

=== CONT  TestAccResourceManagementPrivateLinkAssociation_requiresImport
    testcase.go:113: Step 2/2, expected an error but got none

Copy link
Contributor Author

@teowa teowa Oct 20, 2023

Choose a reason for hiding this comment

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

Can I make name as required to solve this? but in this way user needs to manually set a uuid.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @teowa. On second thought, we should set this to just Optional and add a note in the docs that the user should use ignore_changes on name if they want to use the generated uuid

Comment on lines 125 to 133
variable "primary_location" {
default = %q
}
variable "random_string" {
default = %q
}
variable "random_integer" {
default = %d
}
Copy link
Member

Choose a reason for hiding this comment

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

Given that these seem to be only used once, can we remove these and use the data.Locations.Primary, data.RandomString and data.RandomInteger directly where needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks.


managementGroupId, err := commonids.ParseManagementGroupID(config.ManagementGroupId)
if err != nil {
return fmt.Errorf("parse management group id: %+v", err)
Copy link
Member

@catriona-m catriona-m Oct 16, 2023

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("parse management group id: %+v", err)
return err

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, thanks.

@teowa
Copy link
Contributor Author

teowa commented Oct 24, 2023

--- PASS: TestAccResourceManagementPrivateLinkAssociation_generateName (51.15s)
--- PASS: TestAccResourceManagementPrivateLinkAssociation_basic (113.61s)
--- PASS: TestAccResourceManagementPrivateLinkAssociation_requiresImport (115.29s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/resource      115.309s

Copy link
Member

@catriona-m catriona-m left a comment

Choose a reason for hiding this comment

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

Thanks @teowa LGTM!

@catriona-m catriona-m merged commit d492898 into hashicorp:main Oct 24, 2023
23 checks passed
@github-actions github-actions bot added this to the v3.78.0 milestone Oct 24, 2023
catriona-m added a commit that referenced this pull request Oct 24, 2023
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants