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

Fix error when changing route from virtual appliance to any other type #2184

Conversation

draggeta
Copy link
Contributor

@draggeta draggeta commented Oct 30, 2018

There was a problem with changing from virtual appliance to any other type. This was caused by the fact that next_hop_in_ip_address was computed.

The issue occurred in both azurerm_route_table and azurerm_route.

Also made some other changes to make both resources similar.

  • fix resource
  • add tests to check for regression in future

Closes #1352

@ghost ghost added size/M and removed size/XS labels Oct 30, 2018
@draggeta
Copy link
Contributor Author

There are a few linting errors from other resources. Those are optimizations. Should I fix them here or should they be fixed in separate pull requests?

@tombuildsstuff
Copy link
Contributor

@draggeta sorry about the linting errors, they've been fixed via #2190 - I've re-run the build now that's been merged and this now passes 👍

@draggeta
Copy link
Contributor Author

@tombuildsstuff No worries, I was the cause of one of them anyway 😊

@tombuildsstuff tombuildsstuff added this to the 1.19.0 milestone Nov 3, 2018
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 @draggeta

Thanks for this PR - on the whole this looks good to me, if we can update the test to ensure the next_hop_in_ip_address field is set to an empty string (unassigned) then this otherwise LGTM 👍

Thanks!

azurerm/resource_arm_route_test.go Show resolved Hide resolved
azurerm/resource_arm_route_test.go Show resolved Hide resolved
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 @draggeta

Thanks for pushing those changes - this now LGTM 👍 - I'll kick off the tests shortly

Thanks!

```
$ acctests azurerm TestAccAzureRMRoute_update
=== RUN   TestAccAzureRMRoute_update
--- PASS: TestAccAzureRMRoute_update (151.30s)
PASS
ok  	github.com/terraform-providers/terraform-provider-azurerm/azurerm	153.050s
```
@tombuildsstuff
Copy link
Contributor

hey @draggeta

Running the tests I noticed one of them is failing due to the next_hop_in_ip_address field being conditionally set (and so this isn't set to an empty string):

------- Stdout: -------
=== RUN   TestAccAzureRMRoute_update
--- FAIL: TestAccAzureRMRoute_update (76.21s)
	testing.go:538: Step 0 error: Check failed: Check 3/3 error: azurerm_route.test: Attribute 'next_hop_in_ip_address' not found
FAIL

So that we can get this merged I've pushed a commit to fix this - I hope you don't mind!

Thanks!

@tombuildsstuff
Copy link
Contributor

Tests pass:

screenshot 2018-11-07 at 13 29 19

@tombuildsstuff tombuildsstuff merged commit b7c50a0 into hashicorp:master Nov 7, 2018
tombuildsstuff added a commit that referenced this pull request Nov 7, 2018
tombuildsstuff added a commit that referenced this pull request Nov 7, 2018
@draggeta draggeta deleted the fix-changing-route-from-virtual-appliance-to-other-type branch November 18, 2018 22:36
@moisedo
Copy link

moisedo commented Jan 23, 2019

So in version 0.18 it was allowing to have this next_hop_in_ip_address null, but due to the verification it now needs a value if its specified. The big issue with this is, when you have N amount of routes per route table using a MAP, you cannot use a TF resource template anymore, you will have to create 2 templates (one for next_hop_in_ip_address and one without it) Is there another way to tackle this without duplicating code? Thank you. It will also allow me to understand the reason behind mandating a value on this key when is specified.

@ghost
Copy link

ghost commented Mar 5, 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 Mar 5, 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.

Cannot change route from VirtualAppliance to None
3 participants