New Resource: `azurerm_media_service_account` #2711

seushermsft · 2019-01-18T03:54:30Z

This change implements the creation and storage account configuration for Media Services, but doesn't expose other features such as the StreamingEndpoints.

Issue: #2710

seushermsft · 2019-01-18T04:22:42Z

Newly added integration tests pass:

$ make testacc TESTARGS='-run=TestAccAzureRMMediaServicesAccount_singlestorage'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMMediaServicesAccount_singlestorage -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?       github.com/terraform-providers/terraform-provider-azurerm       [no test files]
=== RUN   TestAccAzureRMMediaServicesAccount_singlestorage
=== PAUSE TestAccAzureRMMediaServicesAccount_singlestorage
=== CONT  TestAccAzureRMMediaServicesAccount_singlestorage
--- PASS: TestAccAzureRMMediaServicesAccount_singlestorage (154.81s)
PASS

$ make testacc TESTARGS='-run=TestAccAzureRMMediaServicesAccount_multiplestorage'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccAzureRMMediaServicesAccount_multiplestorage -timeout 180m -ldflags="-X=github.com/terraform-providers/terraform-provider-azurerm/version.ProviderVersion=acc"
?       github.com/terraform-providers/terraform-provider-azurerm       [no test files]
=== RUN   TestAccAzureRMMediaServicesAccount_multiplestorage
=== PAUSE TestAccAzureRMMediaServicesAccount_multiplestorage
=== CONT  TestAccAzureRMMediaServicesAccount_multiplestorage
--- PASS: TestAccAzureRMMediaServicesAccount_multiplestorage (94.99s)

katbyte

Hi @seushermsft,

Thanks for the new resource! It's off to a great start. I've left some mostly minor comments inline and once those are addressed we can get this merged 🙂

azurerm/resource_arm_media_services.go

azurerm/resource_arm_media_services_test.go

examples/media-services/multiple_storage_accounts/main.tf

website/docs/r/media_services.html.markdown

azurerm/resource_arm_media_services_test.go

tombuildsstuff

hey @seushermsft

Thanks for this PR :)

I've taken a look through and left some comments inline but this mostly LGTM - if we can fix those up (and the tests pass) then this should be good to merge 👍

Thanks!

azurerm/provider.go

azurerm/resource_arm_media_services.go

tombuildsstuff · 2019-01-18T17:04:26Z

azurerm/resource_arm_media_services_test.go

+			{
+				Config: generateTemplate_multiplestorage(resourceName, testLocation()),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					checkAccAzureRMMediaServicesAccount("azurerm_media_services.ams", testLocation(), "2"),


can we remove this function and instead check the values persisted in the state? there's an example of this here https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_dev_test_windows_virtual_machine_test.go#L27

checkAccAzureRMMediaServicesAccount uses functions, such as resource.TestCheckResourceAttr internally to check the state. It only exists to remove the duplication since both acceptance tests have the same validation logic with slightly different values (ex: # of storage accounts).

azurerm/config.go

tombuildsstuff · 2019-01-18T17:22:15Z

azurerm/resource_arm_media_services_test.go

+func checkAccAzureRMMediaServicesAccount(resourceName string, location string, storageCount string) resource.TestCheckFunc {
+	// It would be ideal to also validate which storage account was Primary, but that isn't straight forward.
+	// The key for the storage account's Primary flag is non-deterministic (ex: storage_account.1137153885.is_primary) and
+	// isn't based on the storage account name.


heh indeed, that said we should be able to remove this since the import stage verifies the config matches the local state

I suspect I still need this since its validating that the azure_media_services_account code correctly converts the terraform template to an AMS deployment+configuration. I've only ever used terraform in a tutorial, so I'm pretty ignorant in it, but its not clear how this can be removed.

@tombuildsstuff - I disappeared for a week to work on some other stuff. Can you let me know if I'm misunderstanding the two remaining active comments?

website/docs/r/media_services.html.markdown

tingulfsen · 2019-02-07T00:37:48Z

@tombuildsstuff and @katbyte is there any chance you could take a look at this again regarding the latest comment from @seushermsft above? I would love for this PR to be merged, since I need the functionality provided.

dismissing since changes have been pushed

…dia_services

…'Media Services Account'

… Services. Only a single storage account can be marked as is_primary = true

…e tests locally

…rvices storage account configuration

…id is given in the error when multiple storage accounts are marked as primary

…n. Improve formatting

…dia_services to azure_media_services_account

tombuildsstuff

hey @seushermsft

Thanks for pushing the updates to this PR - I've left a few comments inline but this is mostly looking good to me. I hope you don't mind but I'm going to push a couple of commits to fix the comments mentioned below so that we can get this merged 👍

Thanks!

azurerm/resource_arm_media_services_account.go

...ub.com/Azure/azure-sdk-for-go/services/mediaservices/mgmt/2018-07-01/media/accountfilters.go

tombuildsstuff · 2019-02-22T13:21:26Z

azurerm/resource_arm_media_services_account.go

+			"media_service_account_id": {
+				Type:     schema.TypeString,
+				Computed: true,
+			},


in retrospect looks like this field isn't being deserialized correctly; will push a commit to remove this

upstream issue: Azure/azure-sdk-for-go#4124

tombuildsstuff · 2019-02-22T17:08:31Z

Turns out there's a couple of bugs in the Azure API and SDK which mean we're going to have to ship support for this without Tags and the Media Service ID field:

media_service_id isn't deserialized correctly: Media Service Account ID's (not Resource ID's) don't get deserialized correctly Azure/azure-sdk-for-go#4124
tags field isn't always returned: [Media Services] API doesn't return tags element when none exist Azure/azure-rest-api-specs#5249

For the moment I've removed support for these fields - but when these bugs are resolved we should be able to ship support for these.

… an API bug ``` $ acctests-proxy azurerm TestAccAzureRMMediaServicesAccount_basic === RUN TestAccAzureRMMediaServicesAccount_basic === PAUSE TestAccAzureRMMediaServicesAccount_basic === CONT TestAccAzureRMMediaServicesAccount_basic --- FAIL: TestAccAzureRMMediaServicesAccount_basic (128.02s) testing.go:538: Step 0 error: After applying this step, the plan was not empty: DIFF: UPDATE: azurerm_media_services_account.test tags.%: "" => "<computed>" STATE: azurerm_media_services_account.test: ID = /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/190222174721721594/providers/Microsoft.Media/mediaservices/acctestmsarfshu provider = provider.azurerm location = westeurope name = acctestmsarfshu resource_group_name = 190222174721721594 storage_account.# = 1 storage_account.318018769.id = /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/190222174721721594/providers/Microsoft.Storage/storageAccounts/acctestsa1rfshu storage_account.318018769.is_primary = true Dependencies: azurerm_resource_group.test azurerm_storage_account.first azurerm_resource_group.test: ID = /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/190222174721721594 provider = provider.azurerm location = westeurope name = 190222174721721594 tags.% = 0 azurerm_storage_account.first: ID = /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/190222174721721594/providers/Microsoft.Storage/storageAccounts/acctestsa1rfshu provider = provider.azurerm access_tier = account_encryption_source = Microsoft.Storage account_kind = Storage account_replication_type = GRS account_tier = Standard account_type = Standard_GRS enable_blob_encryption = true enable_file_encryption = true enable_https_traffic_only = false identity.# = 0 location = westeurope name = acctestsa1rfshu network_rules.# = 0 primary_access_key = 2De66EJdd5Am14XG8hYjkgpEf8PJLF0QEvY2QOLaaBzHApo32GWGBKrC5zXyQllegPNI5ex17Sgg0BI+jeYrHQ== primary_blob_connection_string = DefaultEndpointsProtocol=https;BlobEndpoint=https://acctestsa1rfshu.blob.core.windows.net/;AccountName=acctestsa1rfshu;AccountKey=2De66EJdd5Am14XG8hYjkgpEf8PJLF0QEvY2QOLaaBzHApo32GWGBKrC5zXyQllegPNI5ex17Sgg0BI+jeYrHQ== primary_blob_endpoint = https://acctestsa1rfshu.blob.core.windows.net/ primary_connection_string = DefaultEndpointsProtocol=https;AccountName=acctestsa1rfshu;AccountKey=2De66EJdd5Am14XG8hYjkgpEf8PJLF0QEvY2QOLaaBzHApo32GWGBKrC5zXyQllegPNI5ex17Sgg0BI+jeYrHQ==;EndpointSuffix=core.windows.net primary_file_endpoint = https://acctestsa1rfshu.file.core.windows.net/ primary_location = westeurope primary_queue_endpoint = https://acctestsa1rfshu.queue.core.windows.net/ primary_table_endpoint = https://acctestsa1rfshu.table.core.windows.net/ resource_group_name = 190222174721721594 secondary_access_key = cT0gpMIavYw5zuG/DAxyaKHXpTyzglMfvRO0kAQFagWBuZIejsz0j4jZKWW6ct8/IvcMwp5Ers0C2pwJCvdGCw== secondary_connection_string = DefaultEndpointsProtocol=https;AccountName=acctestsa1rfshu;AccountKey=cT0gpMIavYw5zuG/DAxyaKHXpTyzglMfvRO0kAQFagWBuZIejsz0j4jZKWW6ct8/IvcMwp5Ers0C2pwJCvdGCw==;EndpointSuffix=core.windows.net secondary_location = northeurope tags.% = 0 Dependencies: azurerm_resource_group.test FAIL FAIL github.com/terraform-providers/terraform-provider-azurerm/azurerm 128.566s ```

tombuildsstuff · 2019-02-22T17:22:08Z

Tests pass:

ghost · 2019-03-08T18:22:13Z

This has been released in version 1.23.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 = "~> 1.23.0"
}
# ... other configuration ...

ghost · 2019-03-25T13:50:28Z

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 added size/XXL dependencies documentation labels Jan 18, 2019

katbyte previously requested changes Jan 18, 2019

View reviewed changes

katbyte reviewed Jan 18, 2019

View reviewed changes

azurerm/resource_arm_media_services_test.go Outdated Show resolved Hide resolved

katbyte reviewed Jan 18, 2019

View reviewed changes

azurerm/resource_arm_media_services_test.go Outdated Show resolved Hide resolved

tombuildsstuff previously requested changes Jan 18, 2019

View reviewed changes

tombuildsstuff added waiting-response service/media labels Jan 18, 2019

tombuildsstuff changed the title ~~Create Terraform Resource for Azure Media Services~~ New Resource: azurerm_media_service_account Jan 18, 2019

ghost removed the waiting-response label Feb 7, 2019

tombuildsstuff added this to the 1.23.0 milestone Feb 8, 2019

tombuildsstuff self-assigned this Feb 15, 2019

seushermsft added 13 commits February 22, 2019 13:17

Create a terraform provider for Azure Media Services named azurerm_me…

864daf0

…dia_services

Add a TODO for validation of AMS name, update 'Cosmos DB Account' to …

2f6f51b

…'Media Services Account'

Add support for multiple storage accounts when provisiong Azure Media…

1170d89

… Services. Only a single storage account can be marked as is_primary = true

Remove commented code

f56f381

Add a test skeleton, but I am unable to run it or any other acceptanc…

e9c3e08

…e tests locally

Add examples for provisioning AMS through terraform

12b6f14

Add documentation for the azurerm_media_services resource

0ad51e4

Make examples more generic to remove my alias from them

cbdd5fa

Add working acceptance tests and a unit test to verify Azure Media Se…

d7922d5

…rvices storage account configuration

Remove old comment about tests not working

f74b11e

Run gofmt -s -w on azurerm/resource_arm_media_services_test.go

d44e1dd

Update flawed test logic and ensure that the correct storage account …

5a5ee5c

…id is given in the error when multiple storage accounts are marked as primary

Add default value for storage_account.is_primary. Update documentatio…

79dab14

…n. Improve formatting

seushermsft and others added 5 commits February 22, 2019 13:17

Add storage account metadata to the terraform import. Rename azure_me…

01d2903

…dia_services to azure_media_services_account

Fix incorrect link to azure_media_service documentation in azurerm.erb

41831e9

Changes from code review

be750ba

renaming the functions

8efcc7f

go mod vendor

c949427

tombuildsstuff approved these changes Feb 22, 2019

View reviewed changes

tombuildsstuff force-pushed the amsTerraform branch from 872bcc5 to c949427 Compare February 22, 2019 12:27

tombuildsstuff reviewed Feb 22, 2019

View reviewed changes

tombuildsstuff force-pushed the amsTerraform branch from 6789e4e to 0cffa30 Compare February 22, 2019 16:14

r/media_service_account: removing the media_service_account_id field

f2619da

tombuildsstuff force-pushed the amsTerraform branch from 0cffa30 to f2619da Compare February 22, 2019 16:28

tombuildsstuff force-pushed the amsTerraform branch from 15f3c45 to e0af2a8 Compare February 22, 2019 17:13

tombuildsstuff added a commit that referenced this pull request Feb 22, 2019

Updating to include #2711

1099d38

tombuildsstuff merged commit 3c5211d into hashicorp:master Feb 22, 2019

ghost locked and limited conversation to collaborators Mar 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_media_service_account` #2711

New Resource: `azurerm_media_service_account` #2711

seushermsft commented Jan 18, 2019

seushermsft commented Jan 18, 2019

katbyte left a comment

tombuildsstuff left a comment

tombuildsstuff Jan 18, 2019

seushermsft Jan 18, 2019

tombuildsstuff Jan 18, 2019

seushermsft Jan 18, 2019

seushermsft Jan 25, 2019

tingulfsen commented Feb 7, 2019

tombuildsstuff left a comment

tombuildsstuff Feb 22, 2019

tombuildsstuff Feb 22, 2019

tombuildsstuff commented Feb 22, 2019

tombuildsstuff commented Feb 22, 2019

ghost commented Mar 8, 2019

ghost commented Mar 25, 2019

New Resource: azurerm_media_service_account #2711

New Resource: azurerm_media_service_account #2711

Conversation

seushermsft commented Jan 18, 2019

seushermsft commented Jan 18, 2019

katbyte left a comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Jan 18, 2019

Choose a reason for hiding this comment

seushermsft Jan 18, 2019

Choose a reason for hiding this comment

tombuildsstuff Jan 18, 2019

Choose a reason for hiding this comment

seushermsft Jan 18, 2019

Choose a reason for hiding this comment

seushermsft Jan 25, 2019

Choose a reason for hiding this comment

tingulfsen commented Feb 7, 2019

tombuildsstuff left a comment

Choose a reason for hiding this comment

tombuildsstuff Feb 22, 2019

Choose a reason for hiding this comment

tombuildsstuff Feb 22, 2019

Choose a reason for hiding this comment

tombuildsstuff commented Feb 22, 2019

tombuildsstuff commented Feb 22, 2019

ghost commented Mar 8, 2019

ghost commented Mar 25, 2019

New Resource: `azurerm_media_service_account` #2711

New Resource: `azurerm_media_service_account` #2711