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

azurerm_api_management: Enable Tenant Access #10475

Merged
merged 5 commits into from
Feb 18, 2021

Conversation

patst
Copy link
Contributor

@patst patst commented Feb 4, 2021

Implementation for #8578 .

Management REST API can be enabled/ disabled and the primary/ secondary Keys can be used as output variables.

Feedback welcome ;-)

@ghost ghost added the size/M label Feb 4, 2021
@patst patst changed the title azurerm_api_managment: Enable Tenant Access azurerm_api_management: Enable Tenant Access Feb 4, 2021
@patst
Copy link
Contributor Author

patst commented Feb 9, 2021

integration test passed in my subscription

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

THanks for the pr @patst - are you sure the tests pass? they all appear to be failing for us with the tenant_access block being updated.

enant_access.#: "1" => "0"

@patst
Copy link
Contributor Author

patst commented Feb 12, 2021

@katbyte thanks for the feedback. Only the "positive test" seems to work.

The problems is to separate between a disabled tenant_access with

tenant_access {
 enabled= false
}

and a missing tenant_access block. The REST API is returning for both cases the same response.

I will try to figure out a solution. I think the flattenApiManagementTenantAccessSettings function is the place to look for a solution.

If you have any advise that would be great as well

add computed to tenant_block to handle situation where is not defined
@patst
Copy link
Contributor Author

patst commented Feb 15, 2021

@katbyte I changed the tenant_access block to Computed to handle the case if it is not set at all.

The tests I executed passed:

TF_ACC=1 go test ./azurerm/internal/services/apimanagement/api_management_resource_test.go -v -run=TestAccApiManagement_customProps -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccApiManagement_customProps
=== PAUSE TestAccApiManagement_customProps
=== CONT  TestAccApiManagement_customProps
--- PASS: TestAccApiManagement_customProps (2985.12s)
PASS
ok      command-line-arguments  2986.535s

TF_ACC=1 go test ./azurerm/internal/services/apimanagement/api_management_resource_test.go -v -run=TestAccApiManagement_tenantAccess -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccApiManagement_tenantAccess
=== PAUSE TestAccApiManagement_tenantAccess
=== CONT  TestAccApiManagement_tenantAccess
--- PASS: TestAccApiManagement_tenantAccess (2593.46s)
PASS

TF_ACC=1 go test ./azurerm/internal/services/apimanagement/api_management_resource_test.go -v -run=TestAccApiManagement_signInSignUpSettings -timeout 120m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
=== RUN   TestAccApiManagement_signInSignUpSettings
=== PAUSE TestAccApiManagement_signInSignUpSettings
=== CONT  TestAccApiManagement_signInSignUpSettings
--- PASS: TestAccApiManagement_signInSignUpSettings (2766.83s)
PASS

I guess the remaining ones will pass as well.

@ghost ghost added the documentation label Feb 16, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @patst - tests are looking better now, just a couple coments more inline to address and this should be good ro merge

add exported properties to docs
@ghost ghost added size/L and removed size/M labels Feb 18, 2021
@katbyte katbyte added this to the v2.49.0 milestone Feb 18, 2021
@katbyte katbyte merged commit b6fea00 into hashicorp:master Feb 18, 2021
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @patst - LGTM

katbyte added a commit that referenced this pull request Feb 18, 2021
@patst patst deleted the 8578-apim-management-tenant-access branch February 18, 2021 20:10
@ghost
Copy link

ghost commented Feb 26, 2021

This has been released in version 2.49.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.49.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Mar 21, 2021

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Mar 21, 2021
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