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

New Resource: Azure Front Door #3933

Merged
merged 56 commits into from
Sep 7, 2019
Merged

New Resource: Azure Front Door #3933

merged 56 commits into from
Sep 7, 2019

Conversation

WodansSon
Copy link
Collaborator

@WodansSon WodansSon commented Jul 26, 2019

Adding support for Azure Frontdoor Service

[fixes: #3186]

@WodansSon WodansSon changed the title [WIP]New Resource: Frontdoor [WIP]New Resource: Azure Front Door Jul 26, 2019
@katbyte katbyte modified the milestones: v1.33.0, v1.34.0 Aug 21, 2019
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 PR @jeffreyCline,

Overall this looks really good, i've left a bunch of mostly minor comments inline that once addressed this should be good to go 👍

website/docs/r/front_door.html.markdown Outdated Show resolved Hide resolved
website/docs/r/front_door.html.markdown Outdated Show resolved Hide resolved
website/docs/r/front_door.html.markdown Outdated Show resolved Hide resolved
website/docs/r/front_door.html.markdown Outdated Show resolved Hide resolved
website/docs/r/front_door.html.markdown Outdated Show resolved Hide resolved
azurerm/resource_arm_front_door.go Outdated Show resolved Hide resolved
azurerm/resource_arm_front_door.go Outdated Show resolved Hide resolved
azurerm/resource_arm_front_door.go Outdated Show resolved Hide resolved
ID: id,
Name: utils.String(name),
FrontendEndpointProperties: &frontdoor.FrontendEndpointProperties{
// ResourceState:
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the point of these comments 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put them there to show which attributes are in the structure that I have omitted from terraform, it made it easier to keep straight in my head when I was comparing the SDK structs to what I implemented within Terraform.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest either removing them or adding a comment to that effect then

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have removed them.

azurerm/resource_arm_front_door.go Outdated Show resolved Hide resolved
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 changes @jeffreyCline,

Going through this once more, the main thing that stands out is the passing multiple parameters down the stack when we could generate the front door ID once and pass it down instead. I think this would clean up the code a bunch and make it easier to follow.

@@ -0,0 +1,85 @@
package helper
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 just move this one up into the front door package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried moving it but go got confused with the path.

@@ -0,0 +1,172 @@
package validate
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 just move this one up into the front door package?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I tried moving it but go got confused with the path.

)

//Frontdoor name must begin with a letter or number, end with a letter or number and may contain only letters, numbers or hyphens.
func FrontDoorName(i interface{}, k string) (_ []string, errors []error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would then become

Suggested change
func FrontDoorName(i interface{}, k string) (_ []string, errors []error) {
func ValidateFrontDoorName(i interface{}, k string) (_ []string, errors []error) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. Fixed.

return nil, errors
}

func BackendPoolRoutingRuleName(i interface{}, k string) (_ []string, errors []error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func BackendPoolRoutingRuleName(i interface{}, k string) (_ []string, errors []error) {
func ValidateBackendPoolRoutingRuleName(i interface{}, k string) (_ []string, errors []error) {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, Fixed.

}
d.Set("friendly_name", properties.FriendlyName)

if frontDoorFrontendEndpoints, err := flattenArmFrontDoorFrontendEndpoint(properties.FrontendEndpoints, resourceGroup, *resp.Name, meta); frontDoorFrontendEndpoints != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be nil checking resp.name here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, fixed.

}

output := make([]map[string]interface{}, 0)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

result["host_header"] = *backendHostHeader
}

if v.EnabledState == frontdoor.Enabled {
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should replace this with:

Suggested change
if v.EnabledState == frontdoor.Enabled {
result["enabled"] = v.EnabledState == frontdoor.Enabled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}
}

//result["web_application_firewall_policy_link"] = flattenArmFrontDoorFrontendEndpointUpdateParameters_webApplicationFirewallPolicyLink(properties.WebApplicationFirewallPolicyLink)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should remove this comment ?

Suggested change
//result["web_application_firewall_policy_link"] = flattenArmFrontDoorFrontendEndpointUpdateParameters_webApplicationFirewallPolicyLink(properties.WebApplicationFirewallPolicyLink)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

}

func expandArmFrontDoorSubResource(subscriptionId string, resourceGroup string, frontDoorName string, resourceType string, resourceName string) *frontdoor.SubResource {
result := frontdoor.SubResource{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of having this function could we just assign frontdoor.SubResource inline so its clear what is going on?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, fixed.

azurerm/resource_arm_front_door.go Show resolved Hide resolved
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 revisions @jeffreyCline, LGTM 👍

@WodansSon
Copy link
Collaborator Author

image

@WodansSon WodansSon merged commit 7c1971d into master Sep 7, 2019
@WodansSon WodansSon deleted the nr_frontdoor branch September 7, 2019 02:28
WodansSon added a commit that referenced this pull request Sep 7, 2019
@ghost
Copy link

ghost commented Sep 18, 2019

This has been released in version 1.34.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 = "~> 1.34.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Oct 7, 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 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked and limited conversation to collaborators Oct 7, 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.

Feature Request: Azure Front Door
3 participants