Terraform Provider Development Program - Technical Review

[cgriggs01]

Hey Team, I have made a second deeper technical review and have feedback outlined below that I’d like to see addressed before we move on to the official migration and release.

• You recently upgraded to use Go Modules, which is great! But we also require that all the providers dependancies are checked into a vendor/ directory within the repository. This can be done by running go mod vendor.

• The .travic.yml config should be upgraded to us Go version "1.11.x" or "1.12.x"

• GNUmakefile is still using the govendor in the vendor-status make target. This target should be removed from the makefile and its call at .travis.yml:19

• In the DNS App schema, the Read, Update, and Delete actions are wrapped with the withExistingResource function but that function does not properly handle the "not found" condition. Instead of returning an error, it should call d.SetId("") to indicate the resource is not present and then return nil.

• Import function is defined in the dns_app resource but is not implemented and the documentation says that import is not supporter. If there is no import functionality, the function delectation should be removed along with the import_citrixitm_dns_app_test.go

• As a debugging best practice, we would like to have [INFO] log message at least 2 per each C/R/U/D - e.g. creating (input), created (output)

• In DNS App you are calling HasChange across all of the attributes and then doing a generic update. HasChange is more for a specific attribute changing that causes a more specific update to occur (ie. adding an ip address or scaling a node). What you have works, but that is not really the intended usage of HasChange which could impact future resources.

• If an error occurs, the provider should return an error message and should not panic. Error messages need to be propagated if they occur in the API calls unless they are an expected error. Read and Update.

• The website documentation is almost perfect. The only feedback would be to add the environment variable names to the provider arguments. As well change that sections name from Configuration Reference --> Argument Reference

Let me know if you have any questions about this feedback. Thanks for all you work on this provider. We look forward to getting this officially released!

Best,
Chris

[cgriggs01]

Hey @jakewan,

Any update on the progress of these items?
We're excited to officially release this provider.
Thanks
Chris

[jakewan]

Hi @cgriggs01 ,

Thanks for all of this feedback. I've been occupied on production support a lot lately. But right now I'm inputting these into our Jira system for tracking and assignment, and producing internal documentation to help other folks here with development and testing of the Terraform provider.

There's a lot of enthusiasm here for it too. I should be able to start moving ahead on these items in the next few days.

Thanks,
Jacob

[jakewan]

Added dependency packages back to the repository via #20.

[jakewan]

Set Go version to 1.12.x in .travis.yml via #21.

[jakewan]

Removed vendor-status Make target and its call from .travis.yml via #21.

[jakewan]

Refactored citrixitm/resource_citrixitm_dns_app.go to remove the withExistingResource function in favor of a more canonical approach, and updated it to ensure d.SetId("") is called and nil returned when an app expected to exist is not found.

Via PR: #22

[jakewan]

Updated website documentation to clarify support for use of terraform import with DNS app resources.

Via PR: #23

[jakewan]

Added standardized output to each C/R/U/D operation. Also added guidance to CONTRIBUTING.md explaining the practice.

Via PR: #24

[jakewan]

Unfortunately, due to recent strategic changes at Citrix, the product line that this Terraform provider was meant to support will no longer be actively developed. Therefore, work on the provider has ceased.

@cgriggs01 Thanks so much for all your help and feedback.