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

Adding Operational Insight Workspace (a.k.a Log Analytics) #331

Merged
merged 28 commits into from
Sep 26, 2017
Merged

Adding Operational Insight Workspace (a.k.a Log Analytics) #331

merged 28 commits into from
Sep 26, 2017

Conversation

TsuyoshiUshio
Copy link
Contributor

I wanted to provide Operational Insight (a.k.a. Log Analytics). So write code for provision it.

If anyone didn't write the Log Analytics part, would you review/merge this, please?

You can use this like this.

provider "azurerm" {
    subscription_id = "YOUR_SUBSCRITPION_ID"
    client_id = "YOUR_CLIENT_ID"
    client_secret = "YOUR_CLIENT_SECRET"
    tenant_id = "YOUR_TENANT_ID"
}


resource "azurerm_resource_group" "test" {
  name     = "acctestRG-01"
  location = "East US"
}

resource "azurerm_operational_insight_workspace" "test" {
  name                = "acctest-01"
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
  sku = "Free"
}

I wrote a whole Acceptance testing, however, I didn't run it because it is expensive to run. If your Ci help it, and occurs error, I'll fix it. I test it locally.

Also, I don't write an documentation. because I'm not sure if it is written by someone. If this pull request is valuable, I'm happy to write the documentation.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Hey @TsuyoshiUshio

Thanks for opening this PR :)

I've taken a look through and left some comments in-line, but this is off to a good start! My main question at this point is around the naming of the resource, is the latest product name for this Operational Insights or Log Analytics? Unfortunately the API documentation confused me and thus I'm unsure.

To answer your questions in-line:

I wrote a whole Acceptance testing, however, I didn't run it because it is expensive to run. If your Ci help it, and occurs error, I'll fix it. I test it locally.

Sure thing, I'll kick off the tests shortly and post the output here :)

Also, I don't write an documentation. because I'm not sure if it is written by someone. If this pull request is valuable, I'm happy to write the documentation.

This Pull Request looks good, so it'd be great to add some documentation for this. In general that involves an entry in the sidebar (./website/azurerm.erb) and a markdown file containing the documentation (./website/docs/r/operational_insight_workspace.html.markdown)

Thanks!

})
}

func TestAccAzureRMOperationalInsightWorkspace_importoptional(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor we'd tend to name this _importOptional - could we update it to match?

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

"github.com/hashicorp/terraform/helper/resource"
)

func TestAccAzureRMOperationalInsightWorkspace_importrequiredOnly(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

minor we'd tend to name this _importRequiredOnly - could we update it to match?

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!

ForceNew: true,
StateFunc: azureRMNormalizeLocation,
},
"resource_group_name": {
Copy link
Contributor

Choose a reason for hiding this comment

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

as of earlier in the week there's now a resourceGroupNameSchema() - could we switch to using this instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

this'll also fix the following test failure:

resource_group_name:  "acctestrg-5147622897607985308" => "acctestRG-5147622897607985308" (forces new resource)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! I did it.

Required: true,
ForceNew: true,
StateFunc: azureRMNormalizeLocation,
},
Copy link
Contributor

Choose a reason for hiding this comment

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

can we swap location over to using the locationSchema? i.e. "location": locationSchema()

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

Required: true,
ForceNew: true,
},
"workspace_id": { // a.k.a. customer_id
Copy link
Contributor

Choose a reason for hiding this comment

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

minor we'd tend to place this comment within the documentation e.g.

Exported attributes:

* `workspace_id` The Workspace ID / Customer ID for the Operational Insight Workspace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just remove this from code and add it on the documentation.


//if read.ID == nil {
// return fmt.Errorf("Cannot read Operational Inight Workspace '%s' (resource group %s) ID", name, resGroup)
//}
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) - is there any reason why we're not re-requesting the resource here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I just forget to remove it. However, I follow your coding style, I mend it.

value := v.(string)

r, _ := regexp.Compile("^[A-Za-z0-9][A-Za-z0-9-]+[A-Za-z0-9]$")
if !r.MatchString(value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't see a max length listed in the API Documentation - is there one in the Portal we should add checks for?

Copy link
Contributor

@piotrgo piotrgo Sep 21, 2017

Choose a reason for hiding this comment

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

Name requirements are shown in the azure portal as a tooltip.

Workspace name should include 4-63 letters, digits or '-'. The '-' shouldn't be the first or the last symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @piotrgo ! I added the validation with test.

`, rInt, location, rInt)
}

func testAccAzureRMOperationalInsightWorkspace_optional(rInt int, location string) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

given this sets all of the fields we'd tend to call this _Complete rather than Optional - as such could we rename this to match the others?

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!

"checksumSHA1": "/TBRYaTTjpsuKRCKH6nvQfSlLOw=",
"path": "github.com/Azure/azure-sdk-for-go/arm/operationalinsights",
"revision": "df4dd90d076ebbf6e87d08d3f00bfac8ff4bde1a",
"revisionTime": "2017-09-06T21:46:31Z"
Copy link
Contributor

Choose a reason for hiding this comment

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

could we update this to pin to v10.3.0-beta of the Azure SDK for Go (to ensure it's consistent with the other vendored SDK's). You can do this by running:

govendor update github.com/Azure/azure-sdk-for-go/arm/operationalinsights@=v10.3.0-beta

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!

"azurerm_network_interface": resourceArmNetworkInterface(),
"azurerm_network_security_group": resourceArmNetworkSecurityGroup(),
"azurerm_network_security_rule": resourceArmNetworkSecurityRule(),
"azurerm_operational_insight_workspace": resourceArmOperationalInsightWorkspaceService(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little confused around the naming of this resource, is the latest product name for this "Operational Insights" or "Log Analytics"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also confused about that. I discuss with the production team. In conclusion, they'd love to go "Log Analytics" for terraform.tf. However, SDK and REST-API is "Operational Insights". I edit my code to fit to it.

@tombuildsstuff
Copy link
Contributor

Hey @TsuyoshiUshio

I've run the tests, here's the sanitised output:

$ acctests azurerm TestAccAzureRMOperationalInsightWorkspace_
=== RUN   TestAccAzureRMOperationalInsightWorkspace_importrequiredOnly
--- FAIL: TestAccAzureRMOperationalInsightWorkspace_importrequiredOnly (122.83s)
	testing.go:434: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		DESTROY/CREATE: azurerm_operational_insight_workspace.test
		  location:             "eastus" => "eastus"
		  name:                 "acctest-5147622897607985308" => "acctest-5147622897607985308"
		  portal_url:           "https://eus.mms.microsoft.com/Account?tenant=0e3e2e88-8caf-41ca-b4da-e3b33b6c52ec&resource=%2fsubscriptions%2f00000000-0000-0000-0000-000000000000%2fresourcegroups%2facctestrg-5147622897607985308%2fproviders%2fmicrosoft.operationalinsights%2fworkspaces%2facctest-5147622897607985308" => "<computed>"
		  primary_shared_key:   "XXXXXX" => "<computed>"
		  resource_group_name:  "acctestrg-5147622897607985308" => "acctestRG-5147622897607985308" (forces new resource)
		  retention_in_days:    "7" => "<computed>"
		  secondary_shared_key: "XXXXXX" => "<computed>"
		  sku:                  "free" => "Free" (forces new resource)
		  tags.%:               "0" => "<computed>"
		  workspace_id:         "823e0e47-018e-4b44-810c-cb0f1fafd6ff" => "<computed>"

		STATE:

		azurerm_operational_insight_workspace.test:
		  ID = /subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/acctestrg-5147622897607985308/providers/microsoft.operationalinsights/workspaces/acctest-5147622897607985308
		  location = eastus
		  name = acctest-5147622897607985308
		  portal_url = https://eus.mms.microsoft.com/Account?tenant=0e3e2e88-8caf-41ca-b4da-e3b33b6c52ec&resource=%2fsubscriptions%2f00000000-0000-0000-0000-000000000000%2fresourcegroups%2facctestrg-5147622897607985308%2fproviders%2fmicrosoft.operationalinsights%2fworkspaces%2facctest-5147622897607985308
		  primary_shared_key = XXXXXX
		  resource_group_name = acctestrg-5147622897607985308
		  retention_in_days = 7
		  secondary_shared_key = XXXXXX
		  sku = free
		  tags.% = 0
		  workspace_id = 823e0e47-018e-4b44-810c-cb0f1fafd6ff

		  Dependencies:
		    azurerm_resource_group.test
		azurerm_resource_group.test:
		  ID = /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-5147622897607985308
		  location = eastus
		  name = acctestRG-5147622897607985308
		  tags.% = 0
=== RUN   TestAccAzureRMOperationalInsightWorkspace_importoptional
--- FAIL: TestAccAzureRMOperationalInsightWorkspace_importoptional (45.35s)
	testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:

		* azurerm_operational_insight_workspace.test: 1 error(s) occurred:

		* azurerm_operational_insight_workspace.test: operationalinsights.WorkspacesClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="BadRequest" Message="The object supplied in the request is invalid"
=== RUN   TestAccAzureRMOperationalInsightWorkspace_requiredOnly
--- FAIL: TestAccAzureRMOperationalInsightWorkspace_requiredOnly (126.65s)
	testing.go:434: Step 0 error: After applying this step, the plan was not empty:

		DIFF:

		DESTROY/CREATE: azurerm_operational_insight_workspace.test
		  location:             "eastus" => "eastus"
		  name:                 "acctest-7520323984285258837" => "acctest-7520323984285258837"
		  portal_url:           "https://eus.mms.microsoft.com/Account?tenant=0e3e2e88-8caf-41ca-b4da-e3b33b6c52ec&resource=%2fsubscriptions%2f00000000-0000-0000-0000-000000000000%2fresourcegroups%2facctestrg-7520323984285258837%2fproviders%2fmicrosoft.operationalinsights%2fworkspaces%2facctest-7520323984285258837" => "<computed>"
		  primary_shared_key:   "XXXXXX" => "<computed>"
		  resource_group_name:  "acctestrg-7520323984285258837" => "acctestRG-7520323984285258837" (forces new resource)
		  retention_in_days:    "7" => "<computed>"
		  secondary_shared_key: "XXXXXX" => "<computed>"
		  sku:                  "free" => "Free" (forces new resource)
		  tags.%:               "0" => "<computed>"
		  workspace_id:         "09f1a614-9207-4724-9181-fdce600082da" => "<computed>"

		STATE:

		azurerm_operational_insight_workspace.test:
		  ID = /subscriptions/00000000-0000-0000-0000-000000000000/resourcegroups/acctestrg-7520323984285258837/providers/microsoft.operationalinsights/workspaces/acctest-7520323984285258837
		  location = eastus
		  name = acctest-7520323984285258837
		  portal_url = https://eus.mms.microsoft.com/Account?tenant=0e3e2e88-8caf-41ca-b4da-e3b33b6c52ec&resource=%2fsubscriptions%2f00000000-0000-0000-0000-000000000000%2fresourcegroups%2facctestrg-7520323984285258837%2fproviders%2fmicrosoft.operationalinsights%2fworkspaces%2facctest-7520323984285258837
		  primary_shared_key = XXXXXX
		  resource_group_name = acctestrg-7520323984285258837
		  retention_in_days = 7
		  secondary_shared_key = XXXXXX
		  sku = free
		  tags.% = 0
		  workspace_id = 09f1a614-9207-4724-9181-fdce600082da

		  Dependencies:
		    azurerm_resource_group.test
		azurerm_resource_group.test:
		  ID = /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/acctestRG-7520323984285258837
		  location = eastus
		  name = acctestRG-7520323984285258837
		  tags.% = 0
=== RUN   TestAccAzureRMOperationalInsightWorkspace_optional
--- FAIL: TestAccAzureRMOperationalInsightWorkspace_optional (56.47s)
	testing.go:434: Step 0 error: Error applying: 1 error(s) occurred:

		* azurerm_operational_insight_workspace.test: 1 error(s) occurred:

		* azurerm_operational_insight_workspace.test: operationalinsights.WorkspacesClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="BadRequest" Message="The object supplied in the request is invalid"
FAIL
exit status 1
FAIL	github.com/terraform-providers/terraform-provider-azurerm/azurerm	351.323s

As posted above - fixing the Schema should fix two of the three issues visible (SKU and Resource Group name) - the final one is an invalid configuration when creating a workspace :)

Thanks!

@tombuildsstuff
Copy link
Contributor

ping @TsuyoshiUshio - have you had a chance to look into this yet? :)

@TsuyoshiUshio
Copy link
Contributor Author

hi @tombuildsstuff ! I appreciate you! Thank you for the great review, I'll work on it on this weekend. Now I'm asking the production team of the Log Analytics about the first question. It is the biggest problem. I had the same question as you. Log Analytics or Operational Insight. They are discussing now. Once I'll get the answer, I'll quickly done this work. :) Or if you need to hurry up this work, I can do it soon. I heard the HashiCorp new announcement. :)

@TsuyoshiUshio
Copy link
Contributor Author

Until now, they like "Operational Insight" than Log Analytics. If they decided, I'll share in here.

@piotrgo
Copy link
Contributor

piotrgo commented Sep 21, 2017

In terms of naming, in the Azure Portal and latest docs it's "Log Analytics", on the other hand "Operational Insights" appears in older articles.

@TsuyoshiUshio
Copy link
Contributor Author

Hi @piotrgo, @tombuildsstuff
I discussed with the Azure Log Analytics team about the name. They decided to go with Azure Log Analytics as the name. I'll use Operational Insights API inside of it, however, the name from the customer will be ...

resource "azurerm_log_analytics" "test" {
  name                = "acctest-01"
  location            = "${azurerm_resource_group.test.location}"
  resource_group_name = "${azurerm_resource_group.test.name}"
  sku = "Free"
}

I'll start reviewing this whole comment you give! Thank you guys!

Talked with the Log Analytics production team, they'd love to go log analytics. I change the name. However, the REST-API name is operational analytics. I change the schema name as log analytics however, keep the source code as operational insights for the consistency of the Azure SDK naming.

Also, I fix the Acceptance testing error. The root cause of the problem is the Retention in days. It is not stated however, we can use 30 - 730. I add validation function and test for that. https://blogs.msdn.microsoft.com/canberrapfe/2017/01/25/change-oms-log-analytics-retention-period-in-the-azure-portal/ The blog says 31 - 730, however 30 is default value it works.
@TsuyoshiUshio
Copy link
Contributor Author

@tombuildsstuff I finished to review all your request and fix the problem of your Acceptance testing issue. Also, I wrote the documentation. Sounds good? If the Acceptance testing fails, please let me know. If it is OK, please merge it. :) Thank you for your great review. I learned a lot.

@TsuyoshiUshio
Copy link
Contributor Author

Ops. I've got travis-ci error. I'll fix it.

@TsuyoshiUshio
Copy link
Contributor Author

@tombuildsstuff I fixed it. :) My review finished.

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Hey @TsuyoshiUshio

Thanks for your continued effort on this PR. From reading the conversation it sounds like naming it Log Analysis makes sense :)

Taking a look through, I've identified a couple of minor things - but it'd be great if we could rename the internal usages of Operational Insights to Log Analysis (in both the File Names/Method Signatures/Test Names to reference Log Analysis before merging.

I'll run the updated tests now - but this otherwise looks good :)

Thanks!

@@ -0,0 +1,56 @@
package azurerm
Copy link
Contributor

Choose a reason for hiding this comment

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

given the resource has been renamed - can we rename this file to import_arm_log_analytics_workspace_test.go?

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!

"github.com/hashicorp/terraform/helper/resource"
)

func TestAccAzureRMOperationalInsightWorkspace_importRequiredOnly(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we rename this to LogAnalytics?

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!

Create: resourceArmOperationalInsightWorkspaceCreateUpdate,
Read: resourceArmOperationalInsightWorkspaceRead,
Update: resourceArmOperationalInsightWorkspaceCreateUpdate,
Delete: resourceArmOperationalInsightWorkspaceDelete,
Copy link
Contributor

Choose a reason for hiding this comment

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

as above - could we rename this to LogAnalytics?

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


r, _ := regexp.Compile("^[A-Za-z0-9][A-Za-z0-9-]+[A-Za-z0-9]$")
if !r.MatchString(value) {
errors = append(errors, fmt.Errorf("Workspace Name can only contain alphabet, number, and '-' charactor. You can not use '-' as the start and end of the name"))
Copy link
Contributor

Choose a reason for hiding this comment

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

minor and not a blocker charactor -> character

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Done

Type: schema.TypeInt,
Optional: true,
Computed: true,
ValidateFunc: validateAzureRmOperationalInsightWorkspaceRetentionInDays,
Copy link
Contributor

Choose a reason for hiding this comment

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

we should be able to swap this out for:

ValidateFunc: validation.IntBetween(30, 730)

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


The following arguments are supported:

* `name` - (Required) Specifies the name of the Workspace. Changing this forces a new resource to be created. Workspace name should include 4-63 letters, digits or '-'. The '-' shouldn't be the first or the last symbol.
Copy link
Contributor

Choose a reason for hiding this comment

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

can we update this so that Changing this forces a new resource to be created. is the last sentence here?

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!

* PerNode
* Standalone
* Premium
* Unlimited
Copy link
Contributor

Choose a reason for hiding this comment

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

minor we'd tend to quote these to make them highlighted, if possible using these quotes :)

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!

<li<%= sidebar_current("docs-azurerm-oms") %>>
<a href="#">OMS Resources</a>
<ul class="nav nav-visible">
<li<%= sidebar_current("docs-azurerm-log-analytics") %>>
Copy link
Contributor

Choose a reason for hiding this comment

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

these strings are used for the highlight, so they need to be prefixed with the name above - as such could we make this: docs-azurerm-oms-log-analytics?

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!

---
layout: "azurerm"
page_title: "Azure Resource Manager: azurerm_log_analytics"
sidebar_current: "docs-azurerm-resource-log-analytics"
Copy link
Contributor

Choose a reason for hiding this comment

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

(as above) - could we include -oms- in this - to be able to highlight both the title and the resource name in the sidebar?

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

d.Set("resource_group_name", resGroup)
d.Set("workspace_id", resp.CustomerID)
d.Set("portal_url", resp.PortalURL)
d.Set("sku", resp.Sku.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

could we add a check to ensure that the returned SKU object isn't nil here? (this can happen when the Swagger doesn't match the API response, when the SDK is upgraded)

if sku := resp.Sku; sku != nil {
  d.Set("sku", sku.Name)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I didn't know that! Good info!

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

d.Set("workspace_id", resp.CustomerID)
d.Set("portal_url", resp.PortalURL)
if sku := resp.Sku; sku != nil {
d.Set("sku", resp.Sku.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

this can actually become just sku.Name rather resp.Sku.Name - since we're assigning the variable to null check 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.

Good Point! I fix it. :)

@TsuyoshiUshio
Copy link
Contributor Author

Hi @tombuildsstuff, I've done the second review. :) Thank you for the kind advice. :) I hope this PR helps. :)

@tombuildsstuff
Copy link
Contributor

@TsuyoshiUshio I've pushed one commit to make the Resource Group Name case insensitive, but the tests now pass: :)

screen shot 2017-09-26 at 16 58 19


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

* `sku` - (Required) A `sku` block as defined below.
Copy link
Contributor

Choose a reason for hiding this comment

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

@TsuyoshiUshio heads up that as this isn't a separate block, I've pushed a commit to in-line these options :)

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

Hey @TsuyoshiUshio

Thanks for your continued effort on this - I've taken another look through and pushed a couple of minor commits (I hope you don't mind!) - but this now LGTM :)

Thanks!

@TsuyoshiUshio
Copy link
Contributor Author

@tombuildsstuff Thank you! :)

@tombuildsstuff
Copy link
Contributor

@TsuyoshiUshio in-fact researching this before merging I noticed one minor thing - are you opposed to naming this resource azurerm_log_analytics_workspace rather than azurerm_log_analytics? If so I can push a commit to update that :)

@TsuyoshiUshio
Copy link
Contributor Author

TsuyoshiUshio commented Sep 26, 2017

@tombuildsstuff No. I agree with you. azurerm_log_analytics_workspace much more makes sense to me. (I'm not a native speaking guy so my sense of English is not good. ;)

@tombuildsstuff
Copy link
Contributor

@TsuyoshiUshio no problem at all - I've pushed those changes - great work on this :)

@tombuildsstuff
Copy link
Contributor

Tests pass:
screen shot 2017-09-26 at 18 26 12

@tombuildsstuff tombuildsstuff merged commit 9a3a748 into hashicorp:master Sep 26, 2017
tombuildsstuff added a commit that referenced this pull request Sep 26, 2017
@TsuyoshiUshio
Copy link
Contributor Author

@tombuildsstuff Thank you for your kind review! I learned a lot and super excited! :) I appreciate you!

@ghost
Copy link

ghost commented Apr 1, 2020

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 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
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