Reverting the change to make Nested security rules from List to Set. … #893
Conversation
…This had caused regression 455
VaijanathB
requested review from
paultyng,
tombuildsstuff,
metacpp and
JunyiYi
There was a problem hiding this comment.
hey @VaijanathB
Thanks for this PR - I've taken a look through and left a couple of comments in-line but this mostly LGTM. If we can sort the two minor issues this should be good to merge.
Thanks!
| buf.WriteString(fmt.Sprintf("%s-", m["destination_address_prefix"].(string))) | ||
| buf.WriteString(fmt.Sprintf("%s-", m["access"].(string))) | ||
| buf.WriteString(fmt.Sprintf("%d-", m["priority"].(int))) | ||
| buf.WriteString(fmt.Sprintf("%s-", m["direction"].(string))) |
There was a problem hiding this comment.
since all items are contained within the Set's hash function, we actually don't need to specify this function at all - so can we remove this method? the default set hash function will include all values in the set
There was a problem hiding this comment.
Agreed. removed this function.
| } | ||
|
|
||
| func flattenNetworkSecurityRules(rules *[]network.SecurityRule) []map[string]interface{} { | ||
| result := make([]map[string]interface{}, 0, len(*rules)) |
There was a problem hiding this comment.
there's a crash here is rules is nil - can we make this:
result := make([]map[string]interface{}, 0)
|
I have addressed both of these comments. I also reran all the Network Acceptance test locally and they pass with this change. |
|
I've rebased this - tests pass:
|
|
@VaijanathB has confirmed the upgrade path from both 0.2.x -> this branch & 1.1.2 -> this branch |
|
I don't know where I should comment this, but the docs need to be updated. |
|
@franciscocabral would you mind opening a new issue about that with some more information about what needs to be updated? Thanks! :) |
This had caused regression #455. I have verified that resources created after 0.3.1 also work properly once we revert this changes.
I have locally run all the tests using the following command and all the tests pass.
make testacc TEST=./azurerm TESTARGS="-run=TestAccAzureRMNetwork"
Also I have verified that Issue #455 is resolved when we make this change.