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 add sku_name Consumption_0 #6868

Merged
merged 10 commits into from
Dec 21, 2020

Conversation

yupwei68
Copy link
Contributor

Fix #6730

Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

Hey @yupwei68, this change looks good but we will run into a similar problem as the one listed in #6733 where Consumption_0 doesn't work when specified. We'll need to get that working in this PR before we can merge

@yupwei68
Copy link
Contributor Author

@mbfrahry But Consumption_0 works, and it's in the unit test.

@yupwei68 yupwei68 requested a review from mbfrahry May 13, 2020 01:46
@mbfrahry
Copy link
Member

Whist the unit test passes, I don't believe an acceptance test will. Do you mind adding an acceptance test that provisions the resource with Consumption_0 to confirm that one gets provisioned successfully.

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.

see comments above

@ghost ghost added size/XL and removed size/M labels May 14, 2020
@yupwei68
Copy link
Contributor Author

Hi @mbfrahry ,you're right. Changes have been pushed to get Consumption_0 passed. The tests bellow have passed.

=== RUN   TestAccAzureRMApiManagement_basic
=== PAUSE TestAccAzureRMApiManagement_basic
=== CONT  TestAccAzureRMApiManagement_basic
--- PASS: TestAccAzureRMApiManagement_basic (2534.08s)
=== RUN   TestAccAzureRMApiManagement_requiresImport
=== PAUSE TestAccAzureRMApiManagement_requiresImport
=== CONT  TestAccAzureRMApiManagement_requiresImport
--- PASS: TestAccAzureRMApiManagement_requiresImport (2480.47s)
=== RUN   TestAccAzureRMApiManagement_customProps
=== PAUSE TestAccAzureRMApiManagement_customProps
=== CONT  TestAccAzureRMApiManagement_customProps
--- PASS: TestAccAzureRMApiManagement_customProps (2195.22s)
=== RUN   TestAccAzureRMApiManagement_signInSignUpSettings
=== PAUSE TestAccAzureRMApiManagement_signInSignUpSettings
=== CONT  TestAccAzureRMApiManagement_signInSignUpSettings
--- PASS: TestAccAzureRMApiManagement_signInSignUpSettings (2599.11s)
=== RUN   TestAccAzureRMApiManagement_policy
=== PAUSE TestAccAzureRMApiManagement_policy
=== CONT  TestAccAzureRMApiManagement_policy
--- PASS: TestAccAzureRMApiManagement_policy (2873.39s)
=== RUN   TestAccAzureRMApiManagement_consumption
=== PAUSE TestAccAzureRMApiManagement_consumption
=== CONT  TestAccAzureRMApiManagement_consumption
--- PASS: TestAccAzureRMApiManagement_consumption (177.89s)
=== RUN   TestAccAzureRMApiManagement_identityUserAssigned
=== PAUSE TestAccAzureRMApiManagement_identityUserAssigned
=== CONT  TestAccAzureRMApiManagement_identityUserAssigned
--- PASS: TestAccAzureRMApiManagement_identityUserAssigned (2011.97s)

@ghost ghost removed the waiting-response label May 14, 2020
@yupwei68 yupwei68 requested a review from katbyte May 14, 2020 06:18
Copy link
Member

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hi @yupwei68

Thanks for this PR - I've taken a look through and left some comments inline, but we'll need to update the approach used here slightly to account for removing the blocks in a subsequent update - I've left comments inline with more details

Thanks!

@yupwei68
Copy link
Contributor Author

hi @katbyte @tombuildsstuff Except the open comments, other changes have been pushed. Please continue your reviewing.

=== RUN   TestAccAzureRMApiManagement_basic
=== PAUSE TestAccAzureRMApiManagement_basic
=== CONT  TestAccAzureRMApiManagement_basic
--- PASS: TestAccAzureRMApiManagement_basic (2109.62s)
=== RUN   TestAccAzureRMApiManagement_requiresImport
=== PAUSE TestAccAzureRMApiManagement_requiresImport
=== CONT  TestAccAzureRMApiManagement_requiresImport
--- PASS: TestAccAzureRMApiManagement_requiresImport (2117.30s)
=== RUN   TestAccAzureRMApiManagement_customProps
=== PAUSE TestAccAzureRMApiManagement_customProps
=== CONT  TestAccAzureRMApiManagement_customProps
--- PASS: TestAccAzureRMApiManagement_customProps (2006.74s)
=== RUN   TestAccAzureRMApiManagement_signInSignUpSettings
=== PAUSE TestAccAzureRMApiManagement_signInSignUpSettings
=== CONT  TestAccAzureRMApiManagement_signInSignUpSettings
--- PASS: TestAccAzureRMApiManagement_signInSignUpSettings (2479.38s)
=== RUN   TestAccAzureRMApiManagement_policy
=== PAUSE TestAccAzureRMApiManagement_policy
=== CONT  TestAccAzureRMApiManagement_policy
--- PASS: TestAccAzureRMApiManagement_policy (2197.78s)
=== RUN   TestAccAzureRMApiManagement_consumption
=== PAUSE TestAccAzureRMApiManagement_consumption
=== CONT  TestAccAzureRMApiManagement_consumption
--- PASS: TestAccAzureRMApiManagement_consumption (182.58s)
=== RUN   TestAccAzureRMApiManagement_identityUserAssigned
=== PAUSE TestAccAzureRMApiManagement_identityUserAssigned
=== CONT  TestAccAzureRMApiManagement_identityUserAssigned
--- PASS: TestAccAzureRMApiManagement_identityUserAssigned (2127.51s)

@yupwei68
Copy link
Contributor Author

@tombuildsstuff corresponding changes have been made. Please continue reviewing.

@ghost ghost removed the waiting-response label May 27, 2020
@sschmeck
Copy link
Contributor

sschmeck commented Jun 3, 2020

Hi @katbyte, is your finding regarding the SDK constant waiting for another integration? :)

@MireilleMedhat
Copy link

Hello, can somebody please provide updates for this PR? is it planned to be merged any time soon?
Thank you:)

@sschmeck
Copy link
Contributor

sschmeck commented Sep 1, 2020

The integration is blocked until #7603 is merged, which waits for the v14.2.1 release of Azure/go-autorest.

@jbrailsford
Copy link

@sschmeck I believe the dependent change in go-autorest Azure/go-autorest#540 has been merged now, as such, could we prgoress this? I'm hitting my head against #6730 (comment) :(

@yupwei68
Copy link
Contributor Author

@AviateX14 What we depends on is the go-autorest from tombuildstuff' repo, which currently is blocked by go-sdk, as he says. And go-sdk is blocked by some problems released by some service teams. But now we're approaching to the release of these two. I believe I'll start on this PR soon.

@sven-hoffmann
Copy link

Any news here?

@ghost ghost removed the waiting-response label Oct 5, 2020
@yupwei68 yupwei68 changed the title azurerm_api_management validate sku_name azurerm_api_management add sku_name Consumption_0 Nov 26, 2020
@ghost ghost added size/L and removed size/XL labels Nov 26, 2020
@yupwei68
Copy link
Contributor Author

I've merged the upstream master branch.
All tests have passed.
New test:
=== RUN TestAccAzureRMApiManagement_consumption
=== PAUSE TestAccAzureRMApiManagement_consumption
=== CONT TestAccAzureRMApiManagement_consumption
--- PASS: TestAccAzureRMApiManagement_consumption (183.75s)

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 @yupwei68 - LGTM 👍

@katbyte
Copy link
Collaborator

katbyte commented Dec 11, 2020

@yupwei68 - tests have passed so this is good to merge one the conflicts are resolved

@yupwei68
Copy link
Contributor Author

Thanks @katbyte . I've merged the latest change. Please continue your review.

@WodansSon WodansSon added this to the v2.42.0 milestone Dec 21, 2020
@WodansSon WodansSon merged commit e8f41ac into hashicorp:master Dec 21, 2020
WodansSon added a commit that referenced this pull request Dec 21, 2020
@ghost
Copy link

ghost commented Jan 8, 2021

This has been released in version 2.42.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.42.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jan 20, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Jan 20, 2021
@yupwei68 yupwei68 deleted the wyp-6730 branch June 7, 2021 06:45
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.

TF apply/plan have different constraints for "Consumption" tier for API Management
10 participants