Allow existing powerdns_records to be updated instead of destroyed & recreated

[ag-TJNII]

This PR modifies the powerdns_record provider to not delete and recreate records on argument change. The change is fairly minor, as PowerDNS creates and updates use the same API call this simply uses the existing resourcePDNSRecordCreate function for updates and sets ForceNew to false. I did not set ForceNew: false on set_ptr as I'm not familiar with that functionality, so I err'd on the side of safety by leaving the delete/recreate behavior.

Resolves #77

[mbag]

Hi, thanks for the pull request. Please add tests that confirm this patch works. Thank you.

[mbag]

I just noticed current tests in the master are broken. I will try to fix them, if not possible, I'll revert the commit that introduced problems, then please rebase and add tests, thank you.

[ag-TJNII]

Great, thanks. I noticed testacc failed on master for me but I wasn't sure if it was a actual failure or my environment. Do both make test and make testacc use the tests in powerdns/resource_powerdns_record_test.go or are test definitions needed in multiple locations?

[mbag]

@ag-TJNII they both look for tests in powerdns/*_test.go, they are just wrapping go test. You can run single test case (e.g. if you want to run only test you are currently working on) with following command:

TESTARGS="-run TestAccPDNSZoneMasterSOAAPIEDITUndefined" PDNS_SERVER_URL=http://localhost:8081     PDNS_API_KEY=secret     make testacc

Using -run TestCaseName is standard go way of running single tests. You can also use wildcard to run tests with same prefix, so TestAccPDNSZoneMaster* would run all tests starting with TestAccPDNSZoneMaster.

[ag-TJNII]

So to test updates I need to know the inputs to the resource so I can compare current record state to desired record state to make sure the record is updated. This is going to be very difficult with the current test input as const strings model. To add tests I'm thinking:

• Convert the const testPDNSRecordConfigXXXX strings to functions that return a struct.
• Create a function that returns the Terraform DSL for a given struct
• Change the testAccCheckPDNSRecordExists function to take the record struct and verify that the record contents are correct.
• Create a core test function that takes a config struct and runs the basic record test suite against the record. This will be the hook-in point for the update steps.
• Convert the majority of test functions that perform the same basic tests to use the core test function. This will deduplicate the tests and allow update tests to easily be performed against all test suites.

I've created a branch with the baseline of these changes here: master...ag-TJNII:TJNII-powerdns_record_test_refactor This branch is not done but it does show the pattern I have in mind. make testacc runs well enough to show this is a viable path.

This is a pretty significant refactor. I believe it will improve the tests as it will add record field verification to all record types for both create and update. However I don't want to unilaterally rewrite this suite. What's the team's opinion on this proposal? Is this direction acceptable, or should I rethink my strategy?

[mbag]

Check out Terraform Best practices regarding testing, there is an example on how to test resource update: https://www.terraform.io/docs/extend/best-practices/testing.html#update-test-verify-configuration-changes

[ag-TJNII]

Updated the test suite to verify updates. The test suite update:

• Uses structs with generator functions to represent test config, instead of strings. This allows for easier checking of attributes as well as updates to a single argument for test.
• Deduplicated boilerplate within the per-record functions by creating a testPDNSRecordCommonTestCore() function which handles build out of the actual record test, as well as ttl & record update tests
• Added a testAccCheckPDNSRecordContents() function which checks the current records in PDNS against the desired record config

I modified all the basic record tests to check for updates as well as creation. I do not check the values in the Terraform state using TestCheckResourceAttr() as that function has some fairly large limitations around data types, so instead I focused on checking state in PowerDNS.

This is a fairly large refactor of the test suite, which is generally not a nice thing to do to someone else's code base, however I wasn't able to think of a way to do these tests using the old pattern without a significant amount of duplicated code. Please let me know if you'd prefer the tests to be handled another way. Thanks!

[ag-TJNII]

Also after thinking through the tests & API calls I limited this behavior change to ttl and records, as changing the zone, name, or type is creating a new entity in PDNS.

[mbag]

I do not check the values in the Terraform state using TestCheckResourceAttr() as that function has some fairly large limitations around data types, so instead I focused on checking state in PowerDNS.

Testing state has it's purpose and meaning. Please read https://www.terraform.io/docs/extend/testing/acceptance-tests/teststep.html

I know you mean well, but I cannot accept this pull request as it ignores terraform testing practices.

Testing resource update isn't anything new or unique to this provider, again please check the documentation
https://www.terraform.io/docs/extend/best-practices/testing.html#update-test-verify-configuration-changes

[ag-TJNII]

I have read those documents many times, and unfortunately they don't document how to test updates on complex data types like TypeSets. This is the reason I skipped them in the current commit, I was not ignoring best practice. After searching more I see Terraform allows the records to be accessed by the hash used to index the values internally, but I can't seem to find how to get that hash ID.

I see the zone test has these hash IDs baked in:

resource.TestCheckResourceAttr(resourceName, "masters.1048647934", "2.2.2.2"),

How did you determine that 1048647934 is was the correct hash? The best reference I've found so far says to dump the state and copy them out, but does not say how to do that. I'm having difficulty finding documentation on how to dump state in the context of a Terraform provider test. I would appreciate some guidance on this as you've solved this problem before. Thanks.

[hwshadow]

@ag-TJNII to answer your question 104864793 comes from a CRC32 hashing algorithm in hashcode helper
https://github.com/hashicorp/terraform-plugin-sdk/blob/v1.16.0/helper/hashcode/hashcode.go

demo

package main

import (
    "fmt"
    "hash/crc32"
)

func St(s string) int {
    v := int(crc32.ChecksumIEEE([]byte(s)))
    if v >= 0 {
        return v
    }
    if -v >= 0 {
        return -v
    }
    // v == MinInt
    return 0
}

func main() {
    fmt.Println(St("2.2.2.2;"))
    fmt.Println(St("1.1.1.1:1111;"))
}

1048647934
1686215786

https://play.golang.org/p/vUw7w6Fr-HC

see:
String func HashSchema(schema *Schema) in
github.com/hashicorp/terraform-plugin-sdk/helper/schema/set.go
ref return hashcode.String(buf.String())

func SerializeValueForHash( in
github.com/hashicorp/terraform-plugin-sdk/helper/schema/serialize.go
ref case TypeString

[ag-TJNII]

Okay, thanks to @hwshadow's excellent tip I've added Terraform state attribute checking via the resource.TestCheckResourceAttr() test check function. This test suite now:

• Creates an initial resource in PowerDNS
• Confirms the state of the attributes in PowerDNS
• Confirms the state of the attributes in Terraform state
• Updates TTL & records on the resource
• Confirms the state of the attributes in PowerDNS
• Confirms the state of the attributes in Terraform state

The test suite now implements the best practices implemented in https://www.terraform.io/docs/extend/best-practices/testing.html#update-test-verify-configuration-changes, and overall is a significant upgrade over the current test suite in master which only verifies that a record was created, but does not validate any attributes.

@mbag I believe I've addressed your concerns, please let me know if not. Thanks!

[ag-TJNII]

Hi, @mbag. Have you had a moment to review the changes? I need to begin using these changes so I'd like to prioritize any further work needed.

[ag-TJNII]

@mbag Can I get an approve or feedback on what other changes are needed please? I've addressed your concerns.

[mbag]

hi @ag-TJNII the functionality this PR adds is needed, but the complete rewrite of the tests would make the maintenance harder. I've checked some of the providers and at most they use Sprintf to format config string sitring with changeable parameters. IN that case and what we currenlty have, I can just copy paste the resource definition and test it from terraform config. The code in this pull request would make that harder and optionally add need for debugging the HCL formatting function.

Also the acceptance tests cases are similar to what we have now, meaning noone minds code repetition in test cases.

[ag-TJNII]

@mbag I agree refactoring the tests is a big change, I was very hesitant about it. However, the original "string hcl" test pattern only tested that a record was created, it didn't test any values. This is the biggest reason why I changed from static hcl strings to these objects, so that the test input could be compared with what is in both tf state and in powerdns. This refactor greatly increases the test coverage as it allows the test to verify the actual record contents written into PowerDNS. It also greatly cuts down on the amount of boilerplate code needed to test updates as two very similar but slightly different HCL strings aren't needed. I believe this will make testing much easier and more robust in the long term, and I believe this change was necessary to get the level of test coverage requested for the original change.

Yes, there is a function that generates the HCL that will need to be maintained, but I believe the gains outweigh the losses here.

Please make sure the greatly increased test coverage added here isn't overlooked before you 👎 .

[tacerus]

Hi, any news on this? If the test suite changes are the only concern left, we could split them into a separate PR - though it would be nice to have both the new functionality and the improved test coverage.