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.

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

added azurerm_lb_rule data source for #8271 #8365

Merged

Conversation

alvintang
Copy link
Contributor

@alvintang alvintang commented Sep 6, 2020

Changes in this PR for #8271:

  • Data source for azurerm_lb_rule
  • Test for azurerm_lb_rule data source
  • Documentation for azurerm_lb_rule

Fixes #8271

@alvintang alvintang force-pushed the 8271-azurerm-lb-rule-data-source branch from c4a629f to 0ac3ff4 Compare September 7, 2020 04:31
@alvintang alvintang force-pushed the 8271-azurerm-lb-rule-data-source branch from 0ac3ff4 to 3ca258f Compare September 7, 2020 05:17
@alvintang alvintang marked this pull request as ready for review September 7, 2020 05:45
Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvintang Thanks for this contribution. This is looking great, just a few minor comments and suggestions.


func dataSourceArmLoadBalancerRule() *schema.Resource {
return &schema.Resource{
Read: dataSourceArmLoadBalancerLoadBalancingRulesRead,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: I think we can shorten this to dataSourceArmLoadBalancerRuleRead (also singular name)

ValidateFunc: ValidateArmLoadBalancerRuleName,
},

"resource_group_name": azure.SchemaResourceGroupName(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use SchemaResourceGroupNameForDataSource() for data sources (doesn't set ForceNew)

Suggested change
"resource_group_name": azure.SchemaResourceGroupName(),
"resource_group_name": azure.SchemaResourceGroupNameForDataSource(),

return fmt.Errorf("Load Balancer Rule %q was not found in Load Balancer %q (Resource Group: %q)", name, *loadBalancer.Name, resourceGroup)
}

return fmt.Errorf("Error retrieving Load Balancer %s: %s", name, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can drop the extraneous "Error" prefix

Suggested change
return fmt.Errorf("Error retrieving Load Balancer %s: %s", name, err)
return fmt.Errorf("retrieving Load Balancer %s: %s", name, err)


if props.BackendAddressPool != nil {
if err := d.Set("backend_address_pool_id", props.BackendAddressPool.ID); err != nil {
return fmt.Errorf("Error setting `backend_address_pool_id`: %+v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here

Suggested change
return fmt.Errorf("Error setting `backend_address_pool_id`: %+v", err)
return fmt.Errorf("setting `backend_address_pool_id`: %+v", err)


if props.Probe != nil {
if err := d.Set("probe_id", props.Probe.ID); err != nil {
return fmt.Errorf("Error setting `probe_id`: %+v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and here

Suggested change
return fmt.Errorf("Error setting `probe_id`: %+v", err)
return fmt.Errorf("setting `probe_id`: %+v", err)

Comment on lines 86 to 101
resource_group_name = "${azurerm_resource_group.test.name}"
loadbalancer_id = "${azurerm_lb.test.id}"
}

resource "azurerm_lb_probe" "test" {
name = "%s"
resource_group_name = "${azurerm_resource_group.test.name}"
loadbalancer_id = "${azurerm_lb.test.id}"
protocol = "Tcp"
port = 443
}

resource "azurerm_lb_rule" "test" {
name = "%s"
resource_group_name = "${azurerm_resource_group.test.name}"
loadbalancer_id = "${azurerm_lb.test.id}"
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 use TF 0.12 syntax?

Suggested change
resource_group_name = "${azurerm_resource_group.test.name}"
loadbalancer_id = "${azurerm_lb.test.id}"
}
resource "azurerm_lb_probe" "test" {
name = "%s"
resource_group_name = "${azurerm_resource_group.test.name}"
loadbalancer_id = "${azurerm_lb.test.id}"
protocol = "Tcp"
port = 443
}
resource "azurerm_lb_rule" "test" {
name = "%s"
resource_group_name = "${azurerm_resource_group.test.name}"
loadbalancer_id = "${azurerm_lb.test.id}"
resource_group_name = azurerm_resource_group.test.name
loadbalancer_id = azurerm_lb.test.id
}
resource "azurerm_lb_probe" "test" {
name = "%s"
resource_group_name = azurerm_resource_group.test.name
loadbalancer_id = azurerm_lb.test.id
protocol = "Tcp"
port = 443
}
resource "azurerm_lb_rule" "test" {
name = "%s"
resource_group_name = azurerm_resource_group.test.name
loadbalancer_id = azurerm_lb.test.id

---
subcategory: "Load Balancer"
layout: "azurerm"
page_title: "Azure Resource Manager: Data Source: azurerm_lb_rule"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to specify this is a data source in the title

Suggested change
page_title: "Azure Resource Manager: Data Source: azurerm_lb_rule"
page_title: "Azure Resource Manager: azurerm_lb_rule"


* `name` - (Required) The name of this Load Balancer Rule.

* `resource_group_name` - (Required) The name of the Resource Group where the Load Balancer Rule exists. Changing this forces a new Load Balancer Rule to be created.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `resource_group_name` - (Required) The name of the Resource Group where the Load Balancer Rule exists. Changing this forces a new Load Balancer Rule to be created.
* `resource_group_name` - (Required) The name of the Resource Group where the Load Balancer Rule exists.


* `load_distribution` - Specifies the load balancing distribution type used by the Load Balancer.

* `disable_outbound_snat` - If snat is enabled for this Load Balancer Rule.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `disable_outbound_snat` - If snat is enabled for this Load Balancer Rule.
* `disable_outbound_snat` - If outbound SNAT is enabled for this Load Balancer Rule.


* `backend_port` - The port used for internal connections on the endpoint.

* `enable_floating_ip` - If Floating IPs are enabled for this Load Balncer Rule
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* `enable_floating_ip` - If Floating IPs are enabled for this Load Balncer Rule
* `enable_floating_ip` - If Floating IPs are enabled for this Load Balancer Rule

@alvintang
Copy link
Contributor Author

Thanks for the review @manicminer. Latest commit addresses the comments.

Copy link
Contributor

@manicminer manicminer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @alvintang, LGTM 👍

@manicminer manicminer added this to the v2.27.0 milestone Sep 9, 2020
@manicminer manicminer merged commit 4bceaa1 into hashicorp:master Sep 9, 2020
manicminer added a commit that referenced this pull request Sep 9, 2020
@ghost
Copy link

ghost commented Sep 10, 2020

This has been released in version 2.27.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.27.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Oct 9, 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 👉 [email protected]. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data source for azurerm_lb_rule?
2 participants