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

Nat rule #22

Merged
merged 22 commits into from
Aug 9, 2018
Merged

Nat rule #22

merged 22 commits into from
Aug 9, 2018

Conversation

thetonymaster
Copy link
Contributor

Made a mistake changing the names of the tests on #21, so this will depend on that PR

@katbyte
Copy link
Collaborator

katbyte commented Aug 3, 2018

Will review once #21 is meged

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.

@thetonymaster,

Aside from some minor comments i have left inline this mostly LTGM 👍

return &schema.Resource{
Create: resourceArmLoadBalancerNatRuleCreate,
Read: resourceArmLoadBalancerNatRuleRead,
Update: resourceArmLoadBalancerNatRuleCreate,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would we rename this to resourceArmLoadBalancerNatRuleCreateUpdate as it is doing both?

existingNatRule, existingNatRuleIndex, exists := findLoadBalancerNatRuleByName(loadBalancer, d.Get("name").(string))
if exists {
if d.Get("name").(string) == *existingNatRule.Name {
// this probe is being updated/reapplied remove old copy from the slice
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this should be nat rule not probe


if v, ok := d.GetOk("enable_floating_ip"); ok {
enableFloatingIP := v.(bool)
properties.EnableFloatingIP = utils.Bool(enableFloatingIP)
Copy link
Collaborator

Choose a reason for hiding this comment

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

These can be combined into: properties.EnableFloatingIP = utils.Bool(v.(bool))
`

ID: rule.ID,
}

properties.FrontendIPConfiguration = &feip
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could become:

        properties.FrontendIPConfiguration = &network.SubResource{
			ID: rule.ID,
		}

page_title: "Azure Resource Manager: azurestack_lb_nat_rule"
sidebar_current: "docs-azurestack-resource-loadbalancer-nat-rule"
description: |-
Create a LoadBalancer NAT Rule.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be Manages a


# azurestack_lb_nat_rule

Create a LoadBalancer NAT Rule.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be Manages a

Required: true,
StateFunc: ignoreCaseStateFunc,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we get some validation here?

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.

@thetonymaster,

Thank you for the updates, just a couple comments left. The primary blocking is using the SDK values instead of strings for validating the protocol property.

StateFunc: ignoreCaseStateFunc,
DiffSuppressFunc: ignoreCaseDiffSuppressFunc,
ValidateFunc: validation.StringInSlice([]string{
"Tcp",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we use the values provider by the SDK string(network.TransportProtocolTCP),

"frontend_port": {
Type: schema.TypeInt,
Required: true,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we validate the value is a valid port here?

"backend_port": {
Type: schema.TypeInt,
Required: true,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we validate the value is a valid port here?

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.

Thanks for the updates @thetonymaster, this LGTM now 👍

@katbyte
Copy link
Collaborator

katbyte commented Aug 9, 2018

tests pass:
screen shot 2018-08-09 at 16 04 59

@katbyte katbyte merged commit 6a68dc1 into hashicorp:master Aug 9, 2018
katbyte added a commit that referenced this pull request Aug 9, 2018
@katbyte katbyte added this to the 0.3.0 milestone Aug 13, 2018
@ghost
Copy link

ghost commented Jun 27, 2019

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 and limited conversation to collaborators Jun 27, 2019
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.

2 participants