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

[azurerm_traffic_manager_endpoint ] - supper for custom_header and subnet properties #3655

Merged
merged 22 commits into from
Jul 12, 2019

Conversation

Xiang-ruiqing
Copy link
Contributor

  • Be able to add "Subnet Routing Settings" in Azure Traffic Manager Endpoint
  • A follow-up for the Subnet Routing method for Azure Traffic Manager
  • Can add a list of IP ranges to one endpoint, supporting both CIDR and IP range.

@ghost ghost added the size/L label Jun 14, 2019
@Xiang-ruiqing Xiang-ruiqing changed the title Subnet routing Subnet routing settings to Traffic Manager Endpoints Jun 14, 2019
Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @Xiang-ruiqing

Thanks for this PR :)

Taking a look through this mostly LGTM - if we can fix up the minor comments then this should otherwise be good to merge 👍

Thanks!

azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
@Xiang-ruiqing
Copy link
Contributor Author

Fixed.

@ghost ghost removed the waiting-response label Jun 19, 2019
@Xiang-ruiqing
Copy link
Contributor Author

Ready to be merged.

@ghost ghost added size/XL and removed size/L labels Jun 20, 2019
@Xiang-ruiqing Xiang-ruiqing changed the title Subnet routing settings to Traffic Manager Endpoints Subnet routing and custom header settings to Traffic Manager Endpoints Jun 20, 2019
@Xiang-ruiqing
Copy link
Contributor Author

Xiang-ruiqing commented Jun 24, 2019

@tombuildsstuff New feature added and waiting for review. Is there anything else need to be done?

@Xiang-ruiqing
Copy link
Contributor Author

Able to add custom headers and subnets in string lists

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.

Hi @Xiang-ruiqing,

Thank you for the updates. However i am curious as to why you added the string_header and string_subnet properties. This does not match the API and adds much complexity. Could we revert that change?

Also there are two test failures:


------- Stdout: -------
=== RUN   TestAccAzureRMTrafficManagerEndpoint_updateSubnets
=== PAUSE TestAccAzureRMTrafficManagerEndpoint_updateSubnets
=== CONT  TestAccAzureRMTrafficManagerEndpoint_updateSubnets
--- FAIL: TestAccAzureRMTrafficManagerEndpoint_updateSubnets (104.60s)
    testing.go:568: Step 1 error: Check failed: Check 4/6 error: azurerm_traffic_manager_endpoint.testExternal: Attribute 'subnet.0.first' not found
FAIL
------- Stdout: -------
=== RUN   TestAccAzureRMTrafficManagerEndpoint_updateCustomeHeaders
=== PAUSE TestAccAzureRMTrafficManagerEndpoint_updateCustomeHeaders
=== CONT  TestAccAzureRMTrafficManagerEndpoint_updateCustomeHeaders
--- FAIL: TestAccAzureRMTrafficManagerEndpoint_updateCustomeHeaders (73.02s)
    testing.go:568: Step 0 error: errors during apply:
        
        Error: trafficmanager.EndpointsClient#CreateOrUpdate: Failure responding to request: StatusCode=400 -- Original Error: autorest/azure: Service returned an error. Status=400 Code="BadRequest" Message="Address ranges are blank or global for multiple endpoints: acctestend-external190624231009075719, acctestend-external190624231009075719-2. Only one endpoint in a subnet profile may have a blank or global subnet list."
        
          on /opt/teamcity-agent/temp/buildTmp/tf-test266657800/main.tf line 36:
          (source code not available)
        
        
FAIL

azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
@Xiang-ruiqing
Copy link
Contributor Author

@katbyte The commits are rebased and Test error fixed.

@Xiang-ruiqing
Copy link
Contributor Author

@tombuildsstuff @katbyte

@ghost ghost removed the waiting-response label Jul 5, 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.

Thank you for the revisions @Xiang-ruiqing,

I've given this a more detailed review and left some more comments inline mostly around validation and style. Once those have been addressed (and the tests pass) this should be good to merge 🙂

azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
azurerm/resource_arm_traffic_manager_endpoint.go Outdated Show resolved Hide resolved
@ghost ghost added size/XXL and removed size/XL labels Jul 7, 2019
@ghost ghost added size/XL and removed size/XXL labels Jul 7, 2019
@Xiang-ruiqing
Copy link
Contributor Author

@katbyte
Thank you for your time in giving me these suggestions.
I am in summer internship and this PR is part of my project, so if OK, could you merge it?
Therefore, I may finish this project by saying it is in production.

@ghost ghost removed the waiting-response label Jul 7, 2019
@Xiang-ruiqing
Copy link
Contributor Author

@katbyte @tombuildsstuff

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 @Xiang-ruiqing, LGTM now 👍

@katbyte katbyte changed the title Subnet routing and custom header settings to Traffic Manager Endpoints [azurerm_traffic_manager_endpoint ] - supper for custom_header and subnet properties Jul 12, 2019
@katbyte katbyte added this to the v1.32.0 milestone Jul 12, 2019
@katbyte katbyte merged commit 09d4e85 into hashicorp:master Jul 12, 2019
@Xiang-ruiqing
Copy link
Contributor Author

Thank you a lot.

katbyte added a commit that referenced this pull request Jul 12, 2019
@Xiang-ruiqing Xiang-ruiqing deleted the subnet_routing branch July 18, 2019 23:17
@ghost
Copy link

ghost commented Jul 30, 2019

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

@ghost
Copy link

ghost commented Aug 12, 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 Aug 12, 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.

3 participants