-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: adding Azure Log Analytics module #639
feat: adding Azure Log Analytics module #639
Conversation
…rratest into pull-request-632
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.
@skunklab Please take a look a look at review comments.
# --------------------------------------------------------------------------------------------------------------------- | ||
|
||
resource "azurerm_resource_group" "resource_group" { | ||
name = "${var.resource_group_basename}-${var.postfix}" |
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.
switch "${var.resource_group_basename}-${var.postfix}" to "terratest-rg-${var.postfix}"
examples/azure/terraform-azure-loganalytics-example/variables.tf
Outdated
Show resolved
Hide resolved
modules/azure/loganalytics.go
Outdated
return false | ||
} | ||
|
||
return (*ws.Name == workspaceName) |
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.
need to handle when workspace name is invalid. Please follow pattern in AvailabilitySet
func AvailabilitySetExistsE(t testing.TestingT, avsName string, resGroupName string, subscriptionID string) (bool, error) { |
[Microsoft CI Bot] TL;DR; success 👍 You can check the status of the CI Pipeline logs here ; https://github.com/rguthriemsft/terratest/actions/runs/550467385 |
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.
LGTM! Found one missing docs, and https://github.com/gruntwork-io/terratest/pull/639/files#r502750578 still needs to be addressed.
modules/azure/loganalytics.go
Outdated
return string(sku) | ||
} | ||
|
||
func GetLogAnalyticsWorkspaceSkuE(workspaceName string, resourceGroupName string, subscriptionID string) (string, error) { |
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.
NIT: this function needs docs comment.
[Microsoft CI Bot] TL;DR; failure 🤦 You can check the status of the CI Pipeline logs here ; https://github.com/rguthriemsft/terratest/actions/runs/559081496 |
[Microsoft CI Bot] TL;DR; failure 🤦 You can check the status of the CI Pipeline logs here ; https://github.com/rguthriemsft/terratest/actions/runs/559081496 |
[Microsoft CI Bot] TL;DR; success 👍 You can check the status of the CI Pipeline logs here ; https://github.com/rguthriemsft/terratest/actions/runs/559081496 |
@yorinasub17 PR is updated to address the comments. Please review when you have a chance |
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.
LGTM!
#632
Azure loganalytics module for terratest with the following:
(i) Terraform example is examples/azure folder
(ii) loganalytics modules in modules/azure folder
(iii) unit test in test/azure folder
(iv) integration test with example in test/azure folder