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.

Sign up for GitHub

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

Jump to bottom

New data source: azurerm_log_analytics_workspace #1755

Merged
merged 10 commits into from
Aug 14, 2018
Merged

New data source: azurerm_log_analytics_workspace #1755

merged 10 commits into from
Aug 14, 2018

Conversation

TechyMatt
Copy link
Contributor

No description provided.

tombuildsstuff
tombuildsstuff suggested changes Aug 13, 2018
View reviewed changes
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 @mb290

Thanks for this PR - I've left a few minor comments inline but if we can fix those up this otherwise LGTM 👍

Thanks!

azurerm/data_source_log_analytics_workspace.go Outdated

"location": locationForDataSourceSchema(),

"resource_group_name": resourceGroupNameDiffSuppressSchema(),
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 to be resourceGroupNameForDataSourceSchema?

website/docs/d/log_analytics_workspace.html.markdown

* `workspace_id` - The Workspace (or Customer) ID for the Log Analytics Workspace.

* `portal_url` - The Portal URL for the Log Analytics Workspace.
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 also add retention_in_days, sku and tags here?

azurerm/data_source_log_analytics_workspace_test.go Outdated
}

func testAccDataSourceAzureRMLogAnalyticsWorkspace_basicWithDataSource(rInt int, rString string, location string) string {
config := testAccDataSourceAzureRMStorageAccount_basic(rInt, rString, location)
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 could probably switch to using the basic resource from the Resource instead here? e.g. testAccAzureRMLogAnalyticsWorkspace_requiredOnly

@TechyMatt
Copy link
Contributor Author

Hi @tombuildsstuff! Changes have been made. Didn't think/know about being able to leverage the resource from the resource test case so have updated it. Let me know if it's not the right way.

@TechyMatt
Copy link
Contributor Author

@tombuildsstuff after a couple of failed builds and understanding the test harness much more I ended up using

testAccAzureRMLogAnalyticsWorkspace_retentionInDaysComplete

One of my tests was for the number of days retention so I wanted to make sure we aren't expecting MS to keep the same value, instead we can just use what we set. I also updated the expected Sku based on the new test case.

katbyte
katbyte approved these changes Aug 14, 2018
View reviewed changes
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.

@mb290,

Thank you for the requested updates, this LGTM now 👍

@katbyte
Copy link
Collaborator

katbyte commented Aug 14, 2018

Tests pass:

gofmt -w $(find . -name '*.go' |grep -v vendor)
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./azurerm -v -test.run=TestAccDataSourceAzureRMLogAnalyticsWorkspace -timeout 180m
=== RUN   TestAccDataSourceAzureRMLogAnalyticsWorkspace_basic
--- PASS: TestAccDataSourceAzureRMLogAnalyticsWorkspace_basic (91.97s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	92.005s

@katbyte katbyte merged commit 0455931 into hashicorp:master Aug 14, 2018
@ghost
Copy link

ghost commented Mar 30, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@tombuildsstuff tombuildsstuff tombuildsstuff requested changes

@katbyte katbyte katbyte approved these changes

Assignees
No one assigned
Labels
new-data-source service/oms waiting-response
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TechyMatt @katbyte @tombuildsstuff