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

Add application insights data source #2625

Merged
merged 7 commits into from
Jan 10, 2019
Merged

Add application insights data source #2625

merged 7 commits into from
Jan 10, 2019

Conversation

inkel
Copy link
Contributor

@inkel inkel commented Jan 8, 2019

As proposed in #2623 this PR adds a new azurerm_application_insights data source that allows to get the instrumentation key of an existing Application Insights component.

Usage

data "azurerm_application_insights" "myappinsights" {
    name = "my_app_insights_bucket"
    resource_group_name = "${var.resource_group_name}"
}

// Here we use it to inject the AppInsights' instrumentation key in another resource
resource "azurerm_app_service" "myappsvc" {
    # …all other properties…

    app_settings {
        "APPINSIGHTS_INSTRUMENTATIONKEY" = "${data.azurerm_application_insights.myappinsights.instrumentation_key}"
    }
}

inkel added 2 commits January 8, 2019 16:40
This simple data source allows to return the ID and Instrumentation
Key for a given App Insights bucket name.
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.

Hi @inkel,

Thank you for the new data source!

I've noticed you left out the rest of the properties: location, application_type, tags and app_id

Could we add these in? other then that this PR is looking pretty good 🙂

@@ -143,6 +143,7 @@ func Provider() terraform.ResourceProvider {
"azurerm_virtual_machine": dataSourceArmVirtualMachine(),
"azurerm_virtual_network": dataSourceArmVirtualNetwork(),
"azurerm_virtual_network_gateway": dataSourceArmVirtualNetworkGateway(),
"azurerm_application_insights": dataSourceArmApplicationInsights(),
Copy link
Contributor

Choose a reason for hiding this comment

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

minor could we sort this alphabetically to match the rest of this list? it helps when skimming it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point. I did an alphabetical sort of this list and there actually many elements out of order:

diff --git a/azurerm/provider.go b/azurerm/provider.go
index 13b180ea..842c4630 100644
--- a/azurerm/provider.go
+++ b/azurerm/provider.go
@@ -89,27 +89,28 @@ func Provider() terraform.ResourceProvider {
 		},
 
 		DataSourcesMap: map[string]*schema.Resource{
-			"azurerm_azuread_application":                   dataSourceArmAzureADApplication(),
-			"azurerm_azuread_service_principal":             dataSourceArmActiveDirectoryServicePrincipal(),
 			"azurerm_api_management":                        dataSourceApiManagementService(),
-			"azurerm_application_security_group":            dataSourceArmApplicationSecurityGroup(),
-			"azurerm_app_service":                           dataSourceArmAppService(),
 			"azurerm_app_service_plan":                      dataSourceAppServicePlan(),
+			"azurerm_app_service":                           dataSourceArmAppService(),
+			"azurerm_application_insights":                  dataSourceArmApplicationInsights(),
+			"azurerm_application_security_group":            dataSourceArmApplicationSecurityGroup(),
+			"azurerm_azuread_application":                   dataSourceArmAzureADApplication(),
+			"azurerm_azuread_service_principal":             dataSourceArmActiveDirectoryServicePrincipal(),
 			"azurerm_batch_account":                         dataSourceArmBatchAccount(),
 			"azurerm_builtin_role_definition":               dataSourceArmBuiltInRoleDefinition(),
 			"azurerm_cdn_profile":                           dataSourceArmCdnProfile(),
 			"azurerm_client_config":                         dataSourceArmClientConfig(),
-			"azurerm_cosmosdb_account":                      dataSourceArmCosmosDBAccount(),
 			"azurerm_container_registry":                    dataSourceArmContainerRegistry(),
+			"azurerm_cosmosdb_account":                      dataSourceArmCosmosDBAccount(),
 			"azurerm_data_lake_store":                       dataSourceArmDataLakeStoreAccount(),
 			"azurerm_dev_test_lab":                          dataSourceArmDevTestLab(),
 			"azurerm_dns_zone":                              dataSourceArmDnsZone(),
 			"azurerm_eventhub_namespace":                    dataSourceEventHubNamespace(),
 			"azurerm_image":                                 dataSourceArmImage(),
-			"azurerm_key_vault":                             dataSourceArmKeyVault(),
-			"azurerm_key_vault_key":                         dataSourceArmKeyVaultKey(),
 			"azurerm_key_vault_access_policy":               dataSourceArmKeyVaultAccessPolicy(),
+			"azurerm_key_vault_key":                         dataSourceArmKeyVaultKey(),
 			"azurerm_key_vault_secret":                      dataSourceArmKeyVaultSecret(),
+			"azurerm_key_vault":                             dataSourceArmKeyVault(),
 			"azurerm_kubernetes_cluster":                    dataSourceArmKubernetesCluster(),
 			"azurerm_log_analytics_workspace":               dataSourceLogAnalyticsWorkspace(),
 			"azurerm_logic_app_workflow":                    dataSourceArmLogicAppWorkflow(),
@@ -120,8 +121,8 @@ func Provider() terraform.ResourceProvider {
 			"azurerm_monitor_log_profile":                   dataSourceArmMonitorLogProfile(),
 			"azurerm_network_interface":                     dataSourceArmNetworkInterface(),
 			"azurerm_network_security_group":                dataSourceArmNetworkSecurityGroup(),
-			"azurerm_notification_hub":                      dataSourceNotificationHub(),
 			"azurerm_notification_hub_namespace":            dataSourceNotificationHubNamespace(),
+			"azurerm_notification_hub":                      dataSourceNotificationHub(),
 			"azurerm_platform_image":                        dataSourceArmPlatformImage(),
 			"azurerm_public_ip":                             dataSourceArmPublicIP(),
 			"azurerm_public_ips":                            dataSourceArmPublicIPs(),
@@ -143,7 +144,6 @@ func Provider() terraform.ResourceProvider {
 			"azurerm_virtual_machine":                       dataSourceArmVirtualMachine(),
 			"azurerm_virtual_network":                       dataSourceArmVirtualNetwork(),
 			"azurerm_virtual_network_gateway":               dataSourceArmVirtualNetworkGateway(),
-			"azurerm_application_insights":                  dataSourceArmApplicationInsights(),
 		},
 
 		ResourcesMap: map[string]*schema.Resource{

Would you prefer leaving sorting all the lines for another PR and just place azurerm_application_insights where it should be, or should I send the change for all the lines? Sorting all the lines is a bit out of scope of this PR, IMO.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Another PR for this would be great 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll mark this one as resolved and then send a subsequent PR once this gets merged with the sorting. Sounds good?

Copy link
Collaborator

Choose a reason for hiding this comment

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

sounds good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in #2638

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, good point. I did an alphabetical sort of this list and there actually many elements out of order:

@inkel sorry I didn't realise they weren't in order! Thanks for #2638 in any case, I'll take a look now :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem! <3

@tombuildsstuff tombuildsstuff added this to the 1.21.0 milestone Jan 9, 2019
@inkel
Copy link
Contributor Author

inkel commented Jan 9, 2019

Hello @katbyte! Thanks for the review. I didn't include those fields because they didn't look as useful as the instrumentation key (in #2463 I was asked to only return the ID as it was the only useful property for the azurerm_virtual_machine data source), but I'll gladly add those given that you asked 😸

@ghost ghost removed the waiting-response label Jan 9, 2019
@ghost ghost added size/L and removed size/M labels Jan 9, 2019
@inkel
Copy link
Contributor Author

inkel commented Jan 9, 2019

@katbyte as requested I've added the additional attributes in the data source's return object. Let me know what you think!

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.

Thank you for adding those properties @inkel, i've left a couple comments inline that once addressed this PR should be good to merge 🙂

azurerm/data_source_application_insights.go Outdated Show resolved Hide resolved
azurerm/data_source_application_insights.go Outdated Show resolved Hide resolved
azurerm/data_source_application_insights.go Outdated Show resolved Hide resolved
azurerm/data_source_application_insights.go Outdated Show resolved Hide resolved
azurerm/data_source_application_insights.go Outdated Show resolved Hide resolved
@inkel
Copy link
Contributor Author

inkel commented Jan 10, 2019

@katbyte changes applied! Thanks for your time reviewing this 😸

@ghost ghost removed the waiting-response label Jan 10, 2019
@katbyte
Copy link
Collaborator

katbyte commented Jan 10, 2019

Thanks @inkel! LGTM now 🚀

@katbyte katbyte merged commit 97973d3 into hashicorp:master Jan 10, 2019
katbyte added a commit that referenced this pull request Jan 10, 2019
@inkel inkel deleted the feature/appinsights-data-source branch January 10, 2019 09:58
@inkel
Copy link
Contributor Author

inkel commented Jan 10, 2019

Sweet, thanks!!!

@ghost
Copy link

ghost commented Mar 5, 2019

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 and limited conversation to collaborators Mar 5, 2019
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.

3 participants