New resource: azurerm_managed_application #6386

neil-yechenwei · 2020-04-07T09:38:48Z

This PR is the implement of the issue #5294

katbyte

I've left some comments inline detailing some schema changes i'd like to see. I don't think there is any value to converting the param map into json and then back. Lets just keep it as a map for terraform and also generate the target resource group id ourselvs. thanks!

azurerm/internal/services/managedapplications/managed_application_resource.go

katbyte · 2020-04-10T23:44:51Z

azurerm/internal/services/managedapplications/managed_application_resource.go

+		expandedParams, err := structure.ExpandJsonFromString(v.(string))
+		if err != nil {


could we merge these two lines?

Seems I cannot do this since "expandedParams" variable is used in line 163

azurerm/internal/services/managedapplications/managed_application_resource.go

katbyte · 2020-04-15T03:27:18Z

azurerm/internal/services/managedapplications/managed_application_resource.go

+				}, false),
+			},
+
+			"managed_resource_group_id": {


COuld we make this

Suggested change

"managed_resource_group_id": {

"target_resource_group_name": {

and generate the id in code?

katbyte · 2020-04-15T03:28:15Z

azurerm/internal/services/managedapplications/managed_application_resource.go

+			for _, v := range paramsVal {
+				if v != nil {
+					delete(v.(map[string]interface{}), "type")
+				}
+			}
+			json, err := structure.FlattenJsonToString(paramsVal)
+			if err != nil {
+				return fmt.Errorf("failed to serialize JSON from Parameters: %+v", err)
+			}


Given this is a map already we def shouldn't be converting it to json for terraform

neil-yechenwei · 2020-04-16T11:40:04Z

@katbyte , thanks for your comments. I've updated code.

seiffert · 2020-04-17T10:46:40Z

website/docs/r/managed_application.html.markdown

+
+* `kind` - (Required) The kind of the managed application. Possible values are `MarketPlace` and `ServiceCatalog`.
+
+* `target_resource_group_name` - (Required) The name of the target resource group which includes resources managed by Managed Application.


Did you consider naming this managed_resource_group_name? I haven't heard this to be referred to as "target resource group" yet and "managed resource group" would be consistent with az as well:

$ az managedapp create --help Command az managedapp create : Create a managed application. Arguments --kind [Required] : The managed application kind. can be marketplace or servicecatalog. --managed-rg-id -m [Required] : The resource group managed by the managed application. --name -n [Required] : The managed application name. --resource-group -g [Required] : The resource group of the managed application. ...

@katbyte . I think what seiffert suggests is reasonable. I found generally it would be named as "managed_resource_group_name" in Azure side after went through the related docs. I think it would be more friendly to azure users. How do you think? If it's fine, I will update it to "managed_resource_group_name"?

I don't think the API's name is good or clear as to what it is (or if it should be created first) but i'll leave it up to you @neil-yechenwei - please make sure the docs are clear what it means either way

the CLi tool is auto generated from swagger and often carries over tragic names & descriptions @seiffert - we often will change/rename properties so they are more obvious as to what they do ie target resource group implies the resource will act upon that group. However in this case its not as egregous as i've seen elsewhere so i'll leave it up to the PR author.

Thanks both. I updated it as "managed_resource_group_name" and updated the document for the description.

seiffert · 2020-04-17T11:11:27Z

website/docs/r/managed_application.html.markdown

+
+The following attributes are exported:
+
+* `id` - The ID of the Managed Application.


I believe it would be convenient to expose the app's outputs as well. Do you think we could add that from the beginning?

According by kt's previous comments, suggests to not expose outputs from the beginning. Because compute attribute is easily to be added. So suggests to expose them when user asks for. And we don't need to maintain the outputs user doesn't need. And I think all of this app's outputs are unnecessary to user and other resources.

What are the app outputs you would like to see @seiffert and in what instance would they be sueful?

Sorry for the late response. The app outputs can be anything the app publisher decides to output, so this would need to be a generic key/value map.

Outputs could be URLs to a management interface deployed as part of the app or also just configuration values for clients to use the app. E.g. if you would deploy a managed app that contains a database server, the server name could be an output of the app. By making it an attribute of the resource, the server name could be used to provision the databases in that server.

Of course, this functionality can also be developed in a future PR. I was just looking at this for a potential use case of our team where outputs would be good to have.

ahh thanks for explaining that makes a lot of sense, @neil-yechenwei could we get an managed_app_outputs map as suggested?

katbyte

Hi @neil-yechenwei,

Looks like we have twot est failures:

------- Stdout: -------
=== RUN   TestAccAzureRMManagedApplication_requiresImport
=== PAUSE TestAccAzureRMManagedApplication_requiresImport
=== CONT  TestAccAzureRMManagedApplication_requiresImport
--- FAIL: TestAccAzureRMManagedApplication_requiresImport (157.71s)
    testing.go:633: Step 1, expected error:
        
        config is invalid: 2 problems:
        
        - Missing required argument: The argument "kind" is required, but no definition was found.
        - Missing required argument: The argument "managed_resource_group_name" is required, but no definition was found.

and

Test Failed

------- Stdout: -------
=== RUN   TestAccAzureRMManagedApplication_complete
=== PAUSE TestAccAzureRMManagedApplication_complete
=== CONT  TestAccAzureRMManagedApplication_complete
--- FAIL: TestAccAzureRMManagedApplication_complete (79.86s)
    testing.go:640: Step 0 error: errors during apply:
        
        Error: failed to create Managed Application "acctestCompleteManagedApp200421003449886326" (Resource Group "acctestRG-mapp-200421003449886326"): managedapplications.ApplicationsClient#CreateOrUpdate: Failure sending request: StatusCode=400 -- Original Error: Code="ResourcePurchaseValidationFailed" Message="User failed validation to purchase resources. Error message: 'Legal terms have not been accepted for this item on this subscription: '1a6092a6-137e-4025-9a7c-ef77f76f2c02'. To accept legal terms using PowerShell, please use Get-AzureRmMarketplaceTerms and Set-AzureRmMarketplaceTerms API(https://go.microsoft.com/fwlink/?linkid=862451), to accept the terms using Azure CLI, please use az vm image accpet-terms (https://go.microsoft.com/fwlink/?linkid=2110637) or deploy via the Azure portal to accept the terms'"
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test793218955/main.tf line 35:
          (source code not available)

could you update the docs to include an example of how to accept the managed applications terms, and by proxy the command i need to run to enable the tests on our subscription 🙂

katbyte · 2020-04-21T03:27:20Z

website/docs/r/managed_application.html.markdown

+
+* `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created.
+
+* `kind` - (Required) The kind of the managed application. Possible values are `MarketPlace` and `ServiceCatalog`. Changing this forces a new resource to be created.


Suggested change

* `kind` - (Required) The kind of the managed application. Possible values are `MarketPlace` and `ServiceCatalog`. Changing this forces a new resource to be created.

* `kind` - (Required) The kind of the managed application to deply. Possible values are `MarketPlace` and `ServiceCatalog`. Changing this forces a new resource to be created.

katbyte · 2020-04-21T03:27:55Z

website/docs/r/managed_application.html.markdown

+
+* `kind` - (Required) The kind of the managed application. Possible values are `MarketPlace` and `ServiceCatalog`. Changing this forces a new resource to be created.
+
+* `managed_resource_group_name` - (Required) The name of the target resource group which holds all the resources that are required by the managed application. Changing this forces a new resource to be created.


I think this should be

Suggested change

* `managed_resource_group_name` - (Required) The name of the target resource group which holds all the resources that are required by the managed application. Changing this forces a new resource to be created.

* `managed_resource_group_name` - (Required) The name of the target resource group where all the resources deployed by the managed application will reside. Changing this forces a new resource to be created.

katbyte · 2020-04-21T03:28:10Z

website/docs/r/managed_application.html.markdown

+
+* `managed_resource_group_name` - (Required) The name of the target resource group which holds all the resources that are required by the managed application. Changing this forces a new resource to be created.
+
+* `application_definition_id` - (Optional) The fully qualified path of managed application definition ID.


Suggested change

* `application_definition_id` - (Optional) The fully qualified path of managed application definition ID.

* `application_definition_id` - (Optional) The application definition ID to deply.

katbyte · 2020-04-21T03:28:33Z

website/docs/r/managed_application.html.markdown

+
+* `application_definition_id` - (Optional) The fully qualified path of managed application definition ID.
+
+* `parameters` - (Optional) The name and value pairs that define the managed application parameters.


Suggested change

* `parameters` - (Optional) The name and value pairs that define the managed application parameters.

* `parameters` - (Optional) A mapping of name and value pairs to pass to the managed application as parameters.

katbyte · 2020-04-21T03:28:57Z

website/docs/r/managed_application.html.markdown

+
+* `version` - (Required) Specifies the version of the plan from the marketplace.
+
+* `promotion_code` - (Optional) Specifies the promotion code of the plan. 


Suggested change

* `promotion_code` - (Optional) Specifies the promotion code of the plan.

* `promotion_code` - (Optional) Specifies the promotion code to use with the plan.

neil-yechenwei · 2020-04-21T08:22:51Z

@katbyte , thanks for your comments. For test case "TestAccAzureRMManagedApplication_requiresImport" failure, I've fixed. For test case "TestAccAzureRMManagedApplication_complete", I've supplement the description about how to accept terms while plan is specified. So before you run this test case, you have to run command Set-AzContext -SubscriptionId "<insert your subscription id>"; Get-AzMarketplaceTerms -Publisher "cisco" -Product "meraki-vmx" -Name "meraki-vmx100" | Set-AzMarketplaceTerms -Accept first.

My test results:

=== RUN   TestAccAzureRMManagedApplication_requiresImport
=== PAUSE TestAccAzureRMManagedApplication_requiresImport
=== CONT  TestAccAzureRMManagedApplication_requiresImport
--- PASS: TestAccAzureRMManagedApplication_requiresImport (572.98s)
PASS

=== RUN   TestAccAzureRMManagedApplication_complete
=== PAUSE TestAccAzureRMManagedApplication_complete
=== CONT  TestAccAzureRMManagedApplication_complete
--- PASS: TestAccAzureRMManagedApplication_complete (1416.08s)
PASS

katbyte

Thanks @neil-yechenwei, just a couple more comments to address and this should be good 🙂

katbyte · 2020-04-28T22:28:00Z

azurerm/internal/services/managedapplications/managed_application_resource.go

+	}
+
+	if v, ok := d.GetOk("managed_resource_group_name"); ok {
+		targetResourceGroupId := fmt.Sprintf("/subscriptions/%s/resourceGroups/%s", meta.(*clients.Client).Account.SubscriptionId, v)


Is it possible to target a resource group in a different subscription?

Nope. According by previous test results, they must be in the same subscription otherwise it would threw error message which instructs they must be in the same subscription.

cool thanks!

katbyte · 2020-04-28T22:28:21Z

azurerm/internal/services/managedapplications/managed_application_resource.go

+		a := v.(string)
+		b := utils.String(a)
+		parameters.ApplicationDefinitionID = b


could we just do this

Suggested change

a := v.(string)

b := utils.String(a)

parameters.ApplicationDefinitionID = b

parameters.ApplicationDefinitionID = utils.String(v.(string))

katbyte · 2020-04-28T22:29:20Z

azurerm/internal/services/managedapplications/managed_application_resource.go

+		return fmt.Errorf("setting `plan`: %+v", err)
+	}
+	if props := resp.ApplicationProperties; props != nil {
+		parsedManagedResourceGroupID := strings.Split(*props.ManagedResourceGroupID, "/")


could we use the resource group parse id function here maybe?

katbyte · 2020-04-28T22:33:01Z

website/docs/r/managed_application.html.markdown

+
+The following attributes are exported:
+
+* `id` - The ID of the Managed Application.


ahh thanks for explaining that makes a lot of sense, @neil-yechenwei could we get an managed_app_outputs map as suggested?

katbyte · 2020-04-28T22:33:48Z

website/docs/r/managed_application.html.markdown

+
+* `promotion_code` - (Optional) Specifies the promotion code to use with the plan.
+
+~> **NOTE:** When `plan` is specified, legal terms must be accepted for this item on this subscription before creating the Managed Application. For example, to accept the terms using PowerShell, please use `Set-AzContext -SubscriptionId "00000000-0000-0000-0000-000000000000"; Get-AzMarketplaceTerms -Publisher "cisco" -Product "meraki-vmx" -Name "meraki-vmx100" | Set-AzMarketplaceTerms -Accept`.


could we make this a bit more generic

Suggested change

~> **NOTE:** When `plan` is specified, legal terms must be accepted for this item on this subscription before creating the Managed Application. For example, to accept the terms using PowerShell, please use `Set-AzContext -SubscriptionId "00000000-0000-0000-0000-000000000000"; Get-AzMarketplaceTerms -Publisher "cisco" -Product "meraki-vmx" -Name "meraki-vmx100" | Set-AzMarketplaceTerms -Accept`.

~> **NOTE:** When `plan` is specified, legal terms must be accepted for this item on this subscription before creating the Managed Application. For example, to accept the terms using PowerShell, please use `Set-AzContext -SubscriptionId "00000000-0000-0000-0000-000000000000"; Get-AzMarketplaceTerms -Publisher "plan-publisher" -Product "plan-product" -Name "plan-name" | Set-AzMarketplaceTerms -Accept`.

neil-yechenwei · 2020-04-29T03:16:34Z

@katbyte , Thanks for your comments. I've updated code.

katbyte

Thanks @neil-yechenwei, hope you don't mind but i pushed a change to use the marketplae agreement resource as i couldn't get powershell to work 😅

either way LGTM now!

ghost · 2020-05-01T00:08:01Z

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

ghost · 2020-05-30T14:50:42Z

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!

neil-yechenwei added 3 commits March 30, 2020 16:43

New resource & data source: azurerm_managed_application

ae2f536

Update code

cb5c65b

Merge remote-tracking branch 'upstream/master' into mgmt_app

4d382d9

ghost added size/XL documentation labels Apr 7, 2020

magodo added the new-resource label Apr 8, 2020

katbyte requested changes Apr 15, 2020

View reviewed changes

katbyte added waiting-response service/managed-apps labels Apr 15, 2020

Update code

3f703c7

ghost removed the waiting-response label Apr 16, 2020

neil-yechenwei requested a review from katbyte April 16, 2020 11:40

seiffert reviewed Apr 17, 2020

View reviewed changes

Update code

8fae3ac

katbyte requested changes Apr 21, 2020

View reviewed changes

katbyte added the waiting-response label Apr 21, 2020

Update code

83aea0c

ghost removed the waiting-response label Apr 21, 2020

neil-yechenwei requested a review from katbyte April 21, 2020 08:24

katbyte requested changes Apr 28, 2020

View reviewed changes

Update code

f246e92

neil-yechenwei requested a review from katbyte April 29, 2020 03:16

katbyte added this to the v2.8.0 milestone Apr 29, 2020

katbyte added 2 commits April 29, 2020 13:50

use marketplace agreement for the test

360e1c3

udpate docs

57d0440

katbyte approved these changes Apr 29, 2020

View reviewed changes

katbyte added 2 commits April 29, 2020 14:40

Merge remote-tracking branch 'origin/master' into mgmt_app

2f28f28

make terrafmt

95d6556

katbyte merged commit d1e3d76 into hashicorp:master Apr 30, 2020

katbyte added a commit that referenced this pull request Apr 30, 2020

update CHANGELOG.md to include #6386

2fd690e

hashicorp locked and limited conversation to collaborators May 30, 2020

		expandedParams, err := structure.ExpandJsonFromString(v.(string))
		if err != nil {

	"managed_resource_group_id": {
	"target_resource_group_name": {


		* `kind` - (Required) The kind of the managed application. Possible values are `MarketPlace` and `ServiceCatalog`.

		* `target_resource_group_name` - (Required) The name of the target resource group which includes resources managed by Managed Application.


		The following attributes are exported:

		* `id` - The ID of the Managed Application.


		* `location` - (Required) Specifies the supported Azure location where the resource exists. Changing this forces a new resource to be created.

		* `kind` - (Required) The kind of the managed application. Possible values are `MarketPlace` and `ServiceCatalog`. Changing this forces a new resource to be created.


		* `kind` - (Required) The kind of the managed application. Possible values are `MarketPlace` and `ServiceCatalog`. Changing this forces a new resource to be created.

		* `managed_resource_group_name` - (Required) The name of the target resource group which holds all the resources that are required by the managed application. Changing this forces a new resource to be created.


		* `managed_resource_group_name` - (Required) The name of the target resource group which holds all the resources that are required by the managed application. Changing this forces a new resource to be created.

		* `application_definition_id` - (Optional) The fully qualified path of managed application definition ID.


		* `application_definition_id` - (Optional) The fully qualified path of managed application definition ID.

		* `parameters` - (Optional) The name and value pairs that define the managed application parameters.

	* `parameters` - (Optional) The name and value pairs that define the managed application parameters.
	* `parameters` - (Optional) A mapping of name and value pairs to pass to the managed application as parameters.


		* `version` - (Required) Specifies the version of the plan from the marketplace.

		* `promotion_code` - (Optional) Specifies the promotion code of the plan.


		* `promotion_code` - (Optional) Specifies the promotion code to use with the plan.

		~> NOTE: When `plan` is specified, legal terms must be accepted for this item on this subscription before creating the Managed Application. For example, to accept the terms using PowerShell, please use `Set-AzContext -SubscriptionId "00000000-0000-0000-0000-000000000000"; Get-AzMarketplaceTerms -Publisher "cisco" -Product "meraki-vmx" -Name "meraki-vmx100" \| Set-AzMarketplaceTerms -Accept`.

New resource: azurerm_managed_application #6386

New resource: azurerm_managed_application #6386

Conversation

neil-yechenwei commented Apr 7, 2020

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Apr 16, 2020

seiffert Apr 17, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte Apr 20, 2020 • edited

Choose a reason for hiding this comment

neil-yechenwei Apr 20, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Apr 19, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Apr 21, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Apr 21, 2020 • edited

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei Apr 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neil-yechenwei commented Apr 29, 2020

katbyte left a comment

Choose a reason for hiding this comment

ghost commented May 1, 2020

ghost commented May 30, 2020

seiffert Apr 17, 2020 •

edited

katbyte Apr 20, 2020 •

edited

neil-yechenwei Apr 20, 2020 •

edited

neil-yechenwei Apr 19, 2020 •

edited

neil-yechenwei Apr 21, 2020 •

edited

neil-yechenwei commented Apr 21, 2020 •

edited

neil-yechenwei Apr 29, 2020 •

edited