New Resource: `azurerm_logic_app_workflow` #1266

tombuildsstuff · 2018-05-21T16:25:34Z

This PR adds support for Logic Apps. After initially making this a huge self-contained resource, I've pivoted to adding split-out resources for the Actions and Triggers. This allows us to provide better Diff's for specific actions, whilst still allowing for custom Actions and Triggers via the _custom suffixed resources.

Adds the following resources:

* **New Data Source:** `azurerm_logic_app_workflow` [GH-1266]
* **New Resource:** `azurerm_logic_app_action_custom` [GH-1266]
* **New Resource:** `azurerm_logic_app_action_http` [GH-1266]
* **New Resource:** `azurerm_logic_app_trigger_custom` [GH-1266]
* **New Resource:** `azurerm_logic_app_trigger_http_request` [GH-1266]
* **New Resource:** `azurerm_logic_app_trigger_recurrence` [GH-1266]
* **New Resource:** `azurerm_logic_app_workflow` [GH-1266]

Fixes #610

JunyiYi · 2018-05-21T23:14:42Z

azurerm/resource_arm_logic_app_workflow.go

+				MaxItems: 1,
+				Elem: &schema.Resource{
+					Schema: map[string]*schema.Schema{
+						"recurrence": {


There are so many other triggers than recurrence trigger. https://docs.microsoft.com/en-us/azure/logic-apps/logic-apps-workflow-actions-triggers

JunyiYi · 2018-05-22T00:02:16Z

azurerm/resource_arm_logic_app_workflow.go

+				Type:     schema.TypeList,
+				Optional: true,
+				Computed: true,
+				MaxItems: 1,


There could be more than 1 actions.

JunyiYi · 2018-05-22T00:02:49Z

azurerm/resource_arm_logic_app_workflow.go

+				Type:     schema.TypeList,
+				Optional: true,
+				Computed: true,
+				MaxItems: 1,


1 is not enough.

JunyiYi · 2018-05-22T00:03:12Z

azurerm/resource_arm_logic_app_workflow.go

+
+			"resource_group_name": resourceGroupNameSchema(),
+
+			"parameters": {


There are two different parameters here. One is the workflow parameter, the other is the definition parameter.

JunyiYi · 2018-05-22T00:03:59Z

azurerm/resource_arm_logic_app_workflow.go

+
+	_, err := client.CreateOrUpdate(ctx, resourceGroup, name, properties)
+	if err != nil {
+		return err


Error message is not consistent with other return statements. Provide more detailed info.

JunyiYi · 2018-05-22T00:04:10Z

azurerm/resource_arm_logic_app_workflow.go

+
+	read, err := client.Get(ctx, resourceGroup, name)
+	if err != nil {
+		return err


Refine error message here.

azurerm/resource_arm_logic_app_workflow.go

+
+	id, err := parseAzureResourceID(d.Id())
+	if err != nil {
+		return err


azurerm/resource_arm_logic_app_workflow.go

+
+	id, err := parseAzureResourceID(d.Id())
+	if err != nil {
+		return err


dismissing

tombuildsstuff · 2018-07-10T14:07:14Z

Tests pass:

bflad

Some feedback given below, but I do not think anything is blocking 👍

bflad · 2018-07-16T15:33:26Z

azurerm/data_source_logic_app_workflow.go

+	if err != nil {
+		if utils.ResponseWasNotFound(resp.Response) {
+			d.SetId("")
+			return nil


Should this data source be returning an error here?

bflad · 2018-07-16T15:36:19Z

azurerm/data_source_logic_app_workflow_test.go

+				Config: config,
+				Check: resource.ComposeTestCheckFunc(
+					testCheckAzureRMLogicAppWorkflowExists(dataSourceName),
+					resource.TestCheckResourceAttr(dataSourceName, "parameters.%", "0"),


Minor nitpick: should we use resource.TestCheckResourceAttrPair() for these rather than hardcoding the expectations?

that's an enhancement we should make, but across all resources (since a lot of tests follow this pattern); going to skip this for now.

bflad · 2018-07-16T15:39:30Z

azurerm/import_arm_logic_app_action_custom_test.go

+	"github.com/hashicorp/terraform/helper/resource"
+)
+
+func TestAccAzureRMLogicAppActionCustom_importBasic(t *testing.T) {


Point for consideration: In AWS-land, we've been adding the import TestStep directly to the pertinent acceptance tests to reduce:

Indirection/missed tests with having a separate test file

File sprawl (relating to above)

Creating 2x the same resources for testing (potentially more false positives)

I'm not opposed to that approach (we've started doing it for some resources) - but I think that's a larger project outside the scope of this PR - so I'm going to skip this for now.

bflad · 2018-07-16T15:42:32Z

azurerm/logic_apps.go

+	read, err := client.Get(ctx, resourceGroup, logicAppName)
+	if err != nil {
+		if utils.ResponseWasNotFound(read.Response) {
+			d.SetId("")


We should print a log message when removing from state, e.g. log.Printf("[WARN] Logic App (%s) not found, removing from state", d.Id())

on reflection this should be returning an error, since this is the Create method for a sub-resource (updated)

bflad · 2018-07-16T15:44:51Z

azurerm/logic_apps.go

+	}
+
+	if read.WorkflowProperties == nil {
+		return fmt.Errorf("[ERROR] Error parsing Logic App Workflow - `WorkflowProperties` is nil")


Is there a corrective action possible here, even if that means stating that the API response was unexpected?

nope, if this happens the API's returning data that doesn't confirm to the schema - so this is to prevent crashes

bflad · 2018-07-16T15:48:53Z

azurerm/logic_apps_test.go

+			return fmt.Errorf("Not found: %s", name)
+		}
+
+		logicAppId := rs.Primary.Attributes["logic_app_id"]


Should this be coming from rs.Primary.ID?

we could parse it out of that, but this is a Terraform internal ID (from {logicAppID}/{type}/{name}) so I don't think that gains us anything in this instance

bflad · 2018-07-16T15:52:50Z

azurerm/resource_arm_logic_app_trigger_http_request.go

+	return nil
+}
+
+func validateLogicAppTriggerHttpRequestRelativePath(v interface{}, k string) (ws []string, errors []error) {


I think this can be replaced with validation.ValidateRegexp()

so that function checks the string itself is a valid regex, rather than testing for a specific regex; whilst I thought we had a validate method for that I couldn't seem to find it (although I may have missed it)?

bflad · 2018-07-16T15:58:34Z

azurerm/resource_arm_logic_app_workflow.go

+	name := d.Get("name").(string)
+	resourceGroup := d.Get("resource_group_name").(string)
+	location := azureRMNormalizeLocation(d.Get("location").(string))
+	parameters := expandLogicAppWorkflowParameters(d)


I think we generally prefer to be more explicit with our expand methods if possible (making it support map[string]interface{} instead). Passing the raw *schema.ResourceData could introduce subtle maintenance issues, especially if its used across multiple resources.

bflad · 2018-07-16T16:00:41Z

azurerm/resource_arm_logic_app_workflow.go

+		return fmt.Errorf("[ERROR] Error creating Logic App Workflow %q (Resource Group %q): %+v", name, resourceGroup, err)
+	}
+
+	read, err := client.Get(ctx, resourceGroup, name)


I'm guessing this SDK read here is necessary because the SDK create function does not provide the ID -- it feels extraneous to do this and should probably be avoided if possible (e.g. generate the ID from given arguments). The resource read function can catch non-existent IDs.

we do this across the provider since the URI used for Create doesn't always match the URI used for Get, unfortunately

bflad · 2018-07-16T16:03:08Z

azurerm/resource_arm_logic_app_workflow.go

+	resp, err := client.Get(ctx, resourceGroup, name)
+	if err != nil {
+		if utils.ResponseWasNotFound(resp.Response) {
+			d.SetId("")


Same issue here where we should provide logging message when removing resource from state.

Tests pass: ``` $ acctests azurerm TestAccAzureRMLogicAppWorkflow_ === RUN TestAccAzureRMLogicAppWorkflow_empty --- PASS: TestAccAzureRMLogicAppWorkflow_empty (97.38s) === RUN TestAccAzureRMLogicAppWorkflow_tags --- PASS: TestAccAzureRMLogicAppWorkflow_tags (83.56s) === RUN TestAccAzureRMLogicAppWorkflow_actionHTTP --- PASS: TestAccAzureRMLogicAppWorkflow_actionHTTP (75.25s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 256.212s ``` Fixes #610

``` $ acctests azurerm TestAccAzureRMLogicAppWorkflow_ === RUN TestAccAzureRMLogicAppWorkflow_importEmpty --- PASS: TestAccAzureRMLogicAppWorkflow_importEmpty (71.95s) === RUN TestAccAzureRMLogicAppWorkflow_importTags --- PASS: TestAccAzureRMLogicAppWorkflow_importTags (69.51s) === RUN TestAccAzureRMLogicAppWorkflow_empty --- PASS: TestAccAzureRMLogicAppWorkflow_empty (66.77s) === RUN TestAccAzureRMLogicAppWorkflow_tags --- PASS: TestAccAzureRMLogicAppWorkflow_tags (81.51s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 289.789s ```

Tests pass: ``` $ acctests azurerm TestAccAzureRMLogicAppActionHttp_ === RUN TestAccAzureRMLogicAppActionHttp_basic --- PASS: TestAccAzureRMLogicAppActionHttp_basic (65.75s) === RUN TestAccAzureRMLogicAppActionHttp_headers --- PASS: TestAccAzureRMLogicAppActionHttp_headers (61.97s) === RUN TestAccAzureRMLogicAppActionHttp_disappears --- PASS: TestAccAzureRMLogicAppActionHttp_disappears (77.38s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 205.136s ```

…gic_app_trigger_recurrence` - Adding documentation for all of the resources - Making the test check functions more generic

``` $ acctests azurerm TestAccDataSourceAzureRMLogicAppWorkflow_ === RUN TestAccDataSourceAzureRMLogicAppWorkflow_basic --- PASS: TestAccDataSourceAzureRMLogicAppWorkflow_basic (69.73s) === RUN TestAccDataSourceAzureRMLogicAppWorkflow_tags --- PASS: TestAccDataSourceAzureRMLogicAppWorkflow_tags (67.28s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 137.042s ```

tombuildsstuff · 2018-07-17T11:30:26Z

Tests pass:

ghost · 2020-03-30T15:17:43Z

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!

tombuildsstuff added new-resource service/logic labels May 21, 2018

tombuildsstuff requested a review from katbyte May 21, 2018 16:25

tombuildsstuff added this to the 1.6.0 milestone May 21, 2018

tombuildsstuff changed the title ~~New Resource: azurerm_logic_app_workspace~~ New Resource: azurerm_logic_app_workflow May 21, 2018

tombuildsstuff force-pushed the logic-apps branch from b5f3b03 to b6b8d44 Compare May 21, 2018 16:26

JunyiYi reviewed May 21, 2018

View reviewed changes

JunyiYi previously requested changes May 22, 2018

View reviewed changes

tombuildsstuff modified the milestones: 1.6.0, 1.7.0 May 24, 2018

tombuildsstuff modified the milestones: 1.7.0, Soon Jun 1, 2018

tombuildsstuff force-pushed the logic-apps branch from b6b8d44 to 4d77253 Compare July 10, 2018 13:37

tombuildsstuff modified the milestones: Soon, 1.10.0 Jul 10, 2018

tombuildsstuff removed the request for review from katbyte July 10, 2018 13:44

tombuildsstuff force-pushed the logic-apps branch from 4d77253 to 7937447 Compare July 10, 2018 13:51

tombuildsstuff force-pushed the logic-apps branch 2 times, most recently from fb1580c to 44d75bd Compare July 13, 2018 09:51

tombuildsstuff requested a review from a team July 15, 2018 11:42

bflad approved these changes Jul 16, 2018

View reviewed changes

tombuildsstuff added 7 commits July 17, 2018 12:17

Vendoring the Logic App SDK

fb1823c

Upgrading Logic Apps to v18.0.0 of the Azure SDK for Go

c92d324

New Resources: azurerm_logic_app_trigger_http_request / `azurerm_lo…

19f8f31

…gic_app_trigger_recurrence` - Adding documentation for all of the resources - Making the test check functions more generic

New Resource: azurerm_logic_app_action_custom

25e3028

tombuildsstuff added 3 commits July 17, 2018 12:17

New Resource: azurerm_logic_app_trigger_custom

0c03156

Adding an example of how to use a logic app

1496768

tombuildsstuff force-pushed the logic-apps branch from 44d75bd to 70762ae Compare July 17, 2018 10:18

tombuildsstuff added 3 commits July 17, 2018 12:43

Fixing issues raised during code review

60d3e06

Adding a customizeDiff for http requests

706bb7a

Returning an error rather than removing the state

9a5c117

tombuildsstuff merged commit 7a6762b into master Jul 17, 2018

tombuildsstuff deleted the logic-apps branch July 17, 2018 11:34

tombuildsstuff added a commit that referenced this pull request Jul 17, 2018

Updating to include #1266

ae7c41b

tombuildsstuff added a commit that referenced this pull request Jul 17, 2018

Updating to include #1266

b790d5d

ajhall mentioned this pull request Oct 17, 2018

Allow linking a Logic App to an Integration Account #2106

Closed

ghost locked and limited conversation to collaborators Mar 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Resource: `azurerm_logic_app_workflow` #1266

New Resource: `azurerm_logic_app_workflow` #1266

tombuildsstuff commented May 21, 2018 •

edited

Loading

JunyiYi May 21, 2018

JunyiYi May 22, 2018

JunyiYi May 22, 2018

JunyiYi May 22, 2018

JunyiYi May 22, 2018

JunyiYi May 22, 2018

This comment was marked as outdated.

This comment was marked as outdated.

tombuildsstuff commented Jul 10, 2018

bflad left a comment

bflad Jul 16, 2018

bflad Jul 16, 2018

tombuildsstuff Jul 17, 2018 •

edited

Loading

bflad Jul 16, 2018

tombuildsstuff Jul 17, 2018

bflad Jul 16, 2018

tombuildsstuff Jul 17, 2018

bflad Jul 16, 2018

tombuildsstuff Jul 17, 2018

bflad Jul 16, 2018

tombuildsstuff Jul 17, 2018

bflad Jul 16, 2018

tombuildsstuff Jul 17, 2018

bflad Jul 16, 2018

bflad Jul 16, 2018

tombuildsstuff Jul 17, 2018

bflad Jul 16, 2018

tombuildsstuff commented Jul 17, 2018

ghost commented Mar 30, 2020


		"resource_group_name": resourceGroupNameSchema(),

		"parameters": {

New Resource: azurerm_logic_app_workflow #1266

New Resource: azurerm_logic_app_workflow #1266

Conversation

tombuildsstuff commented May 21, 2018 • edited Loading

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

This comment was marked as outdated.

This comment was marked as outdated.

tombuildsstuff commented Jul 10, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff Jul 17, 2018 • edited Loading

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

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

tombuildsstuff commented Jul 17, 2018

ghost commented Mar 30, 2020

New Resource: `azurerm_logic_app_workflow` #1266

New Resource: `azurerm_logic_app_workflow` #1266

tombuildsstuff commented May 21, 2018 •

edited

Loading

tombuildsstuff Jul 17, 2018 •

edited

Loading