Adding Operational Insight Workspace (a.k.a Log Analytics) #331
Conversation
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
minor we'd tend to name this _importOptional - could we update it to match?
| "github.com/hashicorp/terraform/helper/resource" | ||
| ) | ||
|
|
||
| func TestAccAzureRMOperationalInsightWorkspace_importrequiredOnly(t *testing.T) { |
There was a problem hiding this comment.
minor we'd tend to name this _importRequiredOnly - could we update it to match?
| ForceNew: true, | ||
| StateFunc: azureRMNormalizeLocation, | ||
| }, | ||
| "resource_group_name": { |
There was a problem hiding this comment.
as of earlier in the week there's now a resourceGroupNameSchema() - could we switch to using this instead?
There was a problem hiding this comment.
this'll also fix the following test failure:
resource_group_name: "acctestrg-5147622897607985308" => "acctestRG-5147622897607985308" (forces new resource)
There was a problem hiding this comment.
Great! I did it.
| Required: true, | ||
| ForceNew: true, | ||
| StateFunc: azureRMNormalizeLocation, | ||
| }, |
There was a problem hiding this comment.
can we swap location over to using the locationSchema? i.e. "location": locationSchema()
| Required: true, | ||
| ForceNew: true, | ||
| }, | ||
| "workspace_id": { // a.k.a. customer_id |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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) | ||
| //} |
There was a problem hiding this comment.
(as above) - is there any reason why we're not re-requesting the resource here?
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
I can't see a max length listed in the API Documentation - is there one in the Portal we should add checks for?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you @piotrgo ! I added the validation with test.
| `, rInt, location, rInt) | ||
| } | ||
|
|
||
| func testAccAzureRMOperationalInsightWorkspace_optional(rInt int, location string) string { |
There was a problem hiding this comment.
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?
vendor/vendor.json
Outdated
| "checksumSHA1": "/TBRYaTTjpsuKRCKH6nvQfSlLOw=", | ||
| "path": "github.com/Azure/azure-sdk-for-go/arm/operationalinsights", | ||
| "revision": "df4dd90d076ebbf6e87d08d3f00bfac8ff4bde1a", | ||
| "revisionTime": "2017-09-06T21:46:31Z" |
There was a problem hiding this comment.
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
azurerm/provider.go
Outdated
| "azurerm_network_interface": resourceArmNetworkInterface(), | ||
| "azurerm_network_security_group": resourceArmNetworkSecurityGroup(), | ||
| "azurerm_network_security_rule": resourceArmNetworkSecurityRule(), | ||
| "azurerm_operational_insight_workspace": resourceArmOperationalInsightWorkspaceService(), |
There was a problem hiding this comment.
I'm a little confused around the naming of this resource, is the latest product name for this "Operational Insights" or "Log Analytics"?
There was a problem hiding this comment.
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.
|
Hey @TsuyoshiUshio I've run the tests, here's the sanitised output: 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! |
|
ping @TsuyoshiUshio - have you had a chance to look into this yet? :) |
|
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. :) |
|
Until now, they like "Operational Insight" than Log Analytics. If they decided, I'll share in here. |
|
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. |
|
Hi @piotrgo, @tombuildsstuff 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.
|
@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. |
|
Ops. I've got travis-ci error. I'll fix it. |
|
@tombuildsstuff I fixed it. :) My review finished. |
There was a problem hiding this comment.
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 | |||
There was a problem hiding this comment.
given the resource has been renamed - can we rename this file to import_arm_log_analytics_workspace_test.go?
| "github.com/hashicorp/terraform/helper/resource" | ||
| ) | ||
|
|
||
| func TestAccAzureRMOperationalInsightWorkspace_importRequiredOnly(t *testing.T) { |
There was a problem hiding this comment.
can we rename this to LogAnalytics?
| Create: resourceArmOperationalInsightWorkspaceCreateUpdate, | ||
| Read: resourceArmOperationalInsightWorkspaceRead, | ||
| Update: resourceArmOperationalInsightWorkspaceCreateUpdate, | ||
| Delete: resourceArmOperationalInsightWorkspaceDelete, |
There was a problem hiding this comment.
as above - could we rename this to LogAnalytics?
|
|
||
| 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")) |
There was a problem hiding this comment.
minor and not a blocker charactor -> character
| Type: schema.TypeInt, | ||
| Optional: true, | ||
| Computed: true, | ||
| ValidateFunc: validateAzureRmOperationalInsightWorkspaceRetentionInDays, |
There was a problem hiding this comment.
we should be able to swap this out for:
ValidateFunc: validation.IntBetween(30, 730)
|
|
||
| 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. |
There was a problem hiding this comment.
can we update this so that Changing this forces a new resource to be created. is the last sentence here?
| * PerNode | ||
| * Standalone | ||
| * Premium | ||
| * Unlimited |
There was a problem hiding this comment.
minor we'd tend to quote these to make them highlighted, if possible using these quotes :)
website/azurerm.erb
Outdated
| <li<%= sidebar_current("docs-azurerm-oms") %>> | ||
| <a href="#">OMS Resources</a> | ||
| <ul class="nav nav-visible"> | ||
| <li<%= sidebar_current("docs-azurerm-log-analytics") %>> |
There was a problem hiding this comment.
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?
| --- | ||
| layout: "azurerm" | ||
| page_title: "Azure Resource Manager: azurerm_log_analytics" | ||
| sidebar_current: "docs-azurerm-resource-log-analytics" |
There was a problem hiding this comment.
(as above) - could we include -oms- in this - to be able to highlight both the title and the resource name in the sidebar?
| d.Set("resource_group_name", resGroup) | ||
| d.Set("workspace_id", resp.CustomerID) | ||
| d.Set("portal_url", resp.PortalURL) | ||
| d.Set("sku", resp.Sku.Name) |
There was a problem hiding this comment.
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)
}
There was a problem hiding this comment.
Thanks, I didn't know that! Good info!
| d.Set("workspace_id", resp.CustomerID) | ||
| d.Set("portal_url", resp.PortalURL) | ||
| if sku := resp.Sku; sku != nil { | ||
| d.Set("sku", resp.Sku.Name) |
There was a problem hiding this comment.
this can actually become just sku.Name rather resp.Sku.Name - since we're assigning the variable to null check it :)
There was a problem hiding this comment.
Good Point! I fix it. :)
|
Hi @tombuildsstuff, I've done the second review. :) Thank you for the kind advice. :) I hope this PR helps. :) |
|
@TsuyoshiUshio I've pushed one commit to make the Resource Group Name case insensitive, but the tests now pass: :)
|
|
|
||
| * `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. |
There was a problem hiding this comment.
@TsuyoshiUshio heads up that as this isn't a separate block, I've pushed a commit to in-line these options :)
There was a problem hiding this comment.
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!
|
@tombuildsstuff Thank you! :) |
|
@TsuyoshiUshio in-fact researching this before merging I noticed one minor thing - are you opposed to naming this resource |
|
@tombuildsstuff No. I agree with you. |
|
@TsuyoshiUshio no problem at all - I've pushed those changes - great work on this :) |
|
Tests pass: |
|
@tombuildsstuff Thank you for your kind review! I learned a lot and super excited! :) I appreciate you! |
|
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! |
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.
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.