-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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 new application_gateway
data_source
#10268
Adding new application_gateway
data_source
#10268
Conversation
fd74b6a
to
6f57dbe
Compare
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 @petems
Thanks for opening this PR.
I've taken a look through and this is off to a good start - I've taken a look through and left some comments inline, as mentioned offline this'll need to be registered here to be usable in the provider: https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/internal/services/network/registration.go#L24
Thanks!
azurerm/internal/services/network/application_gateway_data_source_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/network/application_gateway_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/network/application_gateway_data_source.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/network/application_gateway_data_source.go
Outdated
Show resolved
Hide resolved
6b196b6
to
bcb5127
Compare
I've pulled most of the logic from the resource, so there's a lot of duplicted code here, is there value in refactoring this into a shared librariy? If so, is there an example out there? |
bcb5127
to
b020e6a
Compare
@petems - we're going to need to flip all the properties to computed as this is a data source |
7fd3af2
to
6ca8c85
Compare
@katbyte done, everything's set as computed except name and Resource Group |
89e7c26
to
38418e1
Compare
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 @petems
Thanks for pushing those changes - I've taken a look through and left some comments inline, if we can get those sorted (and subsequently add/generate some documentation, there's details on how to generate that in the readme) - then we should be able to run the tests here 👍
Thanks!
return err | ||
} | ||
resGroup := id.ResourceGroup | ||
name := id.Path["applicationGateways"] |
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.
whilst this'd work for a Resource (since we have an ID) - it won't work for a Data Source, so this'd want to be:
name := id.Path["applicationGateways"] | |
name := d.Get("name").(string) | |
resourceGroup := d.Get("resource_group_name").(string) |
if utils.ResponseWasNotFound(applicationGateway.Response) { | ||
log.Printf("[DEBUG] Application Gateway %q was not found in Resource Group %q - removing from state", name, resGroup) | ||
d.SetId("") | ||
return nil |
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.
since this is a Data Source rather than a Resource, we should raise an error here when this doesn't exist:
return nil | |
return fmt.Errorf("Application Gateway %q was not found in Resource Group %q", name, resGroup) |
if location := applicationGateway.Location; location != nil { | ||
d.Set("location", azure.NormalizeLocation(*location)) | ||
} | ||
d.Set("zones", applicationGateway.Zones) |
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.
this data source only defines name, location, resource_group_name, tags
above, these other fields don't exist (so will raise an error) - can we remove them, or add the additional fields to the schema?
resource_group_name = azurerm_application_gateway.test.resource_group_name | ||
name = azurerm_application_gateway.test.name | ||
} | ||
`, AppGatewayResource{}.basic(data)) |
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.
sorry, my bad - the type name for this is
`, AppGatewayResource{}.basic(data)) | |
`, ApplicationGatewayResource{}.basic(data)) |
38418e1
to
ea19600
Compare
dismissing since I've pushed changes
Tests look good 👍 |
This has been released in version 2.48.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 2.48.0"
}
# ... other configuration ... |
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! |
Closes #5030
Draft for now, as I want to make sure I'm making the data_source correctly, after that I'll add the rest of the attributes