Creating azure app insights resource #3

lstolyarov · 2017-06-11T13:26:16Z

No description provided.

tombuildsstuff

Thanks for this PR / porting the others over to the new repo :)

I've reviewed and left some comments in-line - but this is off to a good start and the main thing missing is acceptance tests

Thanks!

tombuildsstuff · 2017-06-11T14:09:18Z

azurerm/provider.go

@@ -120,6 +120,8 @@ func Provider() terraform.ResourceProvider {
 			"azurerm_virtual_network":           resourceArmVirtualNetwork(),
 			"azurerm_virtual_network_peering":   resourceArmVirtualNetworkPeering(),

+			"azurerm_app_insights": resourceArmAppInsights(),


can we name this azurerm_application_insights to match the product name?

tombuildsstuff · 2017-06-11T14:09:54Z

azurerm/resource_arm_app_insights.go

+
+	insightProperties := appinsights.ApplicationInsightsComponent{
+		Name:     &name,
+		Type:     &insightsType,


we can remove this, as it's set automatically by Azure

tombuildsstuff · 2017-06-11T14:12:42Z

vendor/github.com/Azure/azure-sdk-for-go/arm/security.tf

@@ -0,0 +1,145 @@
+resource "azurerm_network_security_group" "network_security_group" {


can we remove this file - as I can't see it in the SDK repository?

tombuildsstuff · 2017-06-11T14:14:09Z

vendor/github.com/Azure/azure-sdk-for-go/arm/appinsights/client.go

@@ -0,0 +1,53 @@
+// Package appinsights implements the Azure ARM Appinsights service API version


can we add this using govendor? I believe govendor fetch github.com/Azure/azure-sdk-for-go/arm/appinsights@=v10.0.2-beta should also update the vendor file

tombuildsstuff · 2017-06-12T04:44:42Z

azurerm/resource_arm_app_insights.go

+
+	d.Set("name", name)
+	d.Set("resource_group_name", resGroup)
+	d.Set("instrumentation_key", instrumentationKey)


can we ensure all fields are set here? I've previously looked into this resource and added:

if resp.ApplicationInsightsComponentProperties != nil { d.Set("application_id", resp.ApplicationInsightsComponentProperties.ApplicationID) d.Set("application_type", string(resp.ApplicationInsightsComponentProperties.ApplicationType)) d.Set("instrumentation_key", resp.ApplicationInsightsComponentProperties.InstrumentationKey) }

tombuildsstuff · 2017-06-12T05:09:48Z

azurerm/resource_arm_app_insights.go

+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+func resourceArmAppInsights() *schema.Resource {


can we add acceptance tests and import tests for this resource?

tombuildsstuff · 2017-06-12T05:15:24Z

azurerm/resource_arm_app_insights.go

+				Required: true,
+				ForceNew: true,
+			},
+			"location": locationSchema(),


there's some other fields we should provide here:

"application_id": { Type: schema.TypeString, Required: true, ForceNew: true, }, "application_type": { Type: schema.TypeString, Required: true, ForceNew: true, DiffSuppressFunc: ignoreCaseDiffSuppressFunc, ValidateFunc: validation.StringInSlice([]string{ appinsights.Web, appinsights.Other, ), true), },

tombuildsstuff · 2017-06-12T05:18:53Z

azurerm/resource_arm_app_insights.go

+	kind := d.Get("kind").(string)
+	location := d.Get("location").(string)
+	tags := d.Get("tags").(map[string]interface{})
+	requestSource := "IbizaAIExtension"


From what I can see, this can only be set to:

// RequestSource enumerates the values for request source. type RequestSource string const ( // Rest specifies the rest state for request source. Rest RequestSource = "rest" )

tombuildsstuff · 2017-06-12T05:19:49Z

azurerm/resource_arm_app_insights.go

+		return fmt.Errorf("Error making Read request on Azure app insights %s: %s", name, err)
+	}
+
+	instrumentationKey := resp.ApplicationInsightsComponentProperties.InstrumentationKey


we should also check that ApplicationInsightComponentProperties isn't nil

tombuildsstuff · 2017-06-12T05:21:41Z

azurerm/resource_arm_app_insights.go

+
+	log.Printf("[DEBUG] Deleting app insights %s: %s", resGroup, name)
+
+	_, err = AppInsightsClient.Delete(resGroup, name)


can we check to ensure this has been deleted?

if err != nil { if accResp.StatusCode == http.StatusNotFound { return nil } return fmt.Errorf("Error issuing AzureRM delete request for App Insights '%s': %+v", name, err) }

tombuildsstuff

Hey @lstolyarov

Thanks for this contribution - I've reviewed and left some comments in-line. Seeing as this was so close to completion, I've pushed commits to resolve each of those comments - I hope you don't mind.

Your contribution is still included (and still credited to you), with the appropriate modifications. Feel free to ask about any of the changes.

Tests pass:

$ TF_ACC=1 envchain azurerm go test ./azurerm -v -timeout 300m -parallel 5 -run TestAccAzureRMApplicationInsights
=== RUN   TestAccAzureRMApplicationInsights_importBasicWeb
--- PASS: TestAccAzureRMApplicationInsights_importBasicWeb (50.07s)
=== RUN   TestAccAzureRMApplicationInsights_importBasicOther
--- PASS: TestAccAzureRMApplicationInsights_importBasicOther (51.66s)
=== RUN   TestAccAzureRMApplicationInsights_basicWeb
--- PASS: TestAccAzureRMApplicationInsights_basicWeb (62.12s)
=== RUN   TestAccAzureRMApplicationInsights_basicOther
--- PASS: TestAccAzureRMApplicationInsights_basicOther (52.13s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	216.001s

Thanks!

tombuildsstuff · 2017-06-23T11:07:52Z

azurerm/import_arm_app_insights_test.go

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


could we rename these to ApplicationInsights for consistency?

tombuildsstuff · 2017-06-23T12:32:05Z

azurerm/resource_arm_app_insights.go

+	d.Set("name", name)
+	d.Set("resource_group_name", resGroup)
+	if resp.ApplicationInsightsComponentProperties != nil {
+		d.Set("application_id", resp.ApplicationInsightsComponentProperties.ApplicationID)


From what I can see, ApplicationID appears to be the same as the name in all circumstances - as such I think we should be exposing AppID instead?

tombuildsstuff · 2017-06-23T12:32:40Z

azurerm/resource_arm_app_insights_test.go

+func TestAccAzureRMAppInsights_basic(t *testing.T) {
+
+	ri := acctest.RandInt()
+	config := fmt.Sprintf(testAccAzureRMApplicationInsights_basic, ri, ri)


for newer resources we're tending to make these functions return a formatted strings, i.e. func testAccAzureRMApplicationInsights_basic(ri)

tombuildsstuff · 2017-06-23T12:38:15Z

azurerm/resource_arm_app_insights.go

+		Create: resourceArmApplicationInsightsCreateOrUpdate,
+		Read:   resourceArmApplicationInsightsRead,
+		Update: resourceArmApplicationInsightsCreateOrUpdate,
+		Delete: resourceArmApplicationInsightsDelete,


there's tests for import, but this resource doesn't currently support it - could we add the code to do this?

Importer: &schema.ResourceImporter{ State: schema.ImportStatePassthrough, },

tombuildsstuff · 2017-06-23T12:41:31Z

azurerm/resource_arm_app_insights.go

+	}
+
+	d.Set("name", name)
+	d.Set("resource_group_name", resGroup)


could we also set the location field here:

d.Set("location", azureRMNormalizeLocation(*resp.Location))

tombuildsstuff · 2017-06-23T14:25:49Z

azurerm/resource_arm_app_insights_test.go

+	}
+}
+
+var testAccAzureRMApplicationInsights_basic = `


it'd be good to include a test for the other application_type here too if possible

tombuildsstuff · 2017-06-23T16:48:15Z

azurerm/resource_arm_app_insights.go

+	"github.com/hashicorp/terraform/helper/validation"
+)
+
+func resourceArmApplicationInsights() *schema.Resource {


could we add some documentation for this resource too?

mbfrahry

LGTM

lstolyarov · 2017-06-23T18:00:35Z

@tombuildsstuff that's great. Thanks for the support.

Creating azure app insights resource

Merge from upstream

sync from head

…m_monitor_scheduled_query_rules_log` (#5053) * New Resource: `azurerm_monitor_scheduled_query_rules` * Acceptance tests passing * Fix linting errors; minor cleanup * Minor updates and bug fixes - Fix some sprintf formatting - Improve documentation for cross-resource query - Don't create an empty metric_trigger {} if user doesn't specify block * Remove heredoc in docs example * fix response import * Update tests and docs to pass checks * Add moved tests to registration * First pass update docs post-review * First pass at separating into two resources * Rename action to alert in docs * Fix code review items * Fix lint issues and minor typos * Fix lint issues #2 * Fix lint issues #3 * Fix documentation error * Add monitor helpers * First pass address latest review * Validate metric_trigger query; add example to docs * Address review comments - Fix test crash - Move common functions to helpers - Add more schema validations - Add Update tests * Fix type assertions; simplify cross-resource query example * Review updates - Add metric alerts to LogToMetric tests and docs - Fix type assert from change to TypeList - Fix a couple typos * Add MaxItems for cross-resource query * Address review comments - Improve docs - Use heredoc strings in docs and test queries - Move shared functions into monitor package - Add underscores to test function names * Fix markdown formatting

ghost · 2020-04-01T15:37:23Z

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!

Adding azure app insights resource

bd7b44b

tombuildsstuff suggested changes Jun 12, 2017

View reviewed changes

leostolyarov and others added 8 commits June 12, 2017 20:15

adding vendor dependencies and minor changes

3bf8958

adding vendor dependencies and minor changes

a1bfccd

adding acceptance tests

c2fac3e

making acc tests to pass

36d1680

Fixing up the PR comments

f814fe2

Added tests covering the other application type

ce4c4d9

Renaming app_insights -> application_insights

6860ffe

Adding documentation for Application Insights

39f4b66

tombuildsstuff reviewed Jun 23, 2017

View reviewed changes

tombuildsstuff approved these changes Jun 23, 2017

View reviewed changes

tombuildsstuff requested a review from mbfrahry June 23, 2017 17:04

mbfrahry approved these changes Jun 23, 2017

View reviewed changes

tombuildsstuff added enhancement new-resource labels Jun 23, 2017

tombuildsstuff merged commit 28bbe92 into hashicorp:master Jun 23, 2017

tombuildsstuff added a commit that referenced this pull request Jun 23, 2017

Adding azurerm_application_insights from #3

36b6dc7

tombuildsstuff added a commit that referenced this pull request Jul 4, 2017

Merge pull request #3 from lstolyarov/azurerm/app-insights-resource

45c53ca

Creating azure app insights resource

tombuildsstuff added a commit that referenced this pull request Jul 4, 2017

Adding azurerm_application_insights from #3

b319747

sebastus mentioned this pull request Jul 12, 2017

catch up with the team. (#1) #165

Closed

tombuildsstuff pushed a commit that referenced this pull request Sep 14, 2017

Merge pull request #3 from terraform-providers/master

b66ae9b

Merge from upstream

metacpp mentioned this pull request Nov 29, 2017

Updated the way user agent string gets assigned. #587

Merged

siddthota mentioned this pull request May 31, 2018

Issue with spinning-up SQL Server 2012 running on Windows Server 2012 R2 Datacenter VM? #1325

Closed

EmJayBee83 mentioned this pull request Jul 3, 2018

terraform destroy Failure: Resources deleted out of order #1482

Closed

tombuildsstuff pushed a commit that referenced this pull request Feb 27, 2019

Merge pull request #3 from terraform-providers/master

f63dc24

sync from head

lrxtom2 mentioned this pull request Mar 4, 2019

Demo code and test defect with NetworkInterfaceApplicationGatewayBackendAddressPoolAssociation #2992

Closed

neil-yechenwei mentioned this pull request Sep 16, 2019

Fix TF issue 4151: azurerm_app_service blocks not working as expected #4328

Closed

tombuildsstuff mentioned this pull request Nov 7, 2019

Add kubernetes_cluster_agent_pool #4046

Closed

venkey1000 mentioned this pull request Dec 31, 2019

Updating terraform files from version 1.22 to 1.39 with out changing manually in every file #5253

Closed

mcdafydd added a commit to mcdafydd/terraform-provider-azurerm that referenced this pull request Feb 1, 2020

Fix lint issues hashicorp#3

d2a15de

wyarde mentioned this pull request Feb 27, 2020

Add tags to cluster node pools #5931

Merged

buissjer mentioned this pull request Apr 1, 2020

Support for function_app vnet integration #6267

Closed

ghost locked and limited conversation to collaborators Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Creating azure app insights resource #3

Creating azure app insights resource #3

lstolyarov commented Jun 11, 2017

tombuildsstuff left a comment

tombuildsstuff Jun 11, 2017

tombuildsstuff Jun 11, 2017

tombuildsstuff Jun 11, 2017

tombuildsstuff Jun 11, 2017

tombuildsstuff Jun 12, 2017

tombuildsstuff Jun 12, 2017

tombuildsstuff Jun 12, 2017

tombuildsstuff Jun 12, 2017

tombuildsstuff Jun 12, 2017

tombuildsstuff Jun 12, 2017

tombuildsstuff left a comment

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

tombuildsstuff Jun 23, 2017

mbfrahry left a comment

lstolyarov commented Jun 23, 2017

ghost commented Apr 1, 2020

		@@ -0,0 +1,145 @@
		resource "azurerm_network_security_group" "network_security_group" {

		@@ -0,0 +1,53 @@
		// Package appinsights implements the Azure ARM Appinsights service API version


		log.Printf("[DEBUG] Deleting app insights %s: %s", resGroup, name)

		_, err = AppInsightsClient.Delete(resGroup, name)

Creating azure app insights resource #3

Creating azure app insights resource #3

Conversation

lstolyarov commented Jun 11, 2017

tombuildsstuff 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

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 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

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

mbfrahry left a comment

Choose a reason for hiding this comment

lstolyarov commented Jun 23, 2017

ghost commented Apr 1, 2020