-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
New data source: azurerm_log_analytics_workspace #1755
New data source: azurerm_log_analytics_workspace #1755
Conversation
There was a problem hiding this 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!
|
||
"location": locationForDataSourceSchema(), | ||
|
||
"resource_group_name": resourceGroupNameDiffSuppressSchema(), |
There was a problem hiding this comment.
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
?
|
||
* `workspace_id` - The Workspace (or Customer) ID for the Log Analytics Workspace. | ||
|
||
* `portal_url` - The Portal URL for the Log Analytics Workspace. |
There was a problem hiding this comment.
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?
} | ||
|
||
func testAccDataSourceAzureRMLogAnalyticsWorkspace_basicWithDataSource(rInt int, rString string, location string) string { | ||
config := testAccDataSourceAzureRMStorageAccount_basic(rInt, rString, location) |
There was a problem hiding this comment.
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
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. |
@tombuildsstuff after a couple of failed builds and understanding the test harness much more I ended up using
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. |
There was a problem hiding this 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 👍
Tests pass:
|
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! |
No description provided.