Terraform Provider Development Program, Review 1

[paddycarver]

Hi! My name is Paddy, I'm part of the Terraform Ecosystem team at HashiCorp. I've been assigned to review your provider for possible acceptance into the terraform-providers org and inclusion on terraform.io. I've done a preliminary review of your data sources and the linode_instance resource, and found enough to give feedback on that I felt it was a representative review. I have not looked at the nodebalancers or other resources yet, but my intention is to point out areas that could be improved that are representative, and allow you to apply that feedback to other areas of the codebase, as well. When this part of the feedback has been addressed, I'll take another pass at all the resources.

I do want to start by saying that your code looks great so far, and that you shouldn't be intimidated by the number of issues being raised here. Some are rather important and critical to get right, but you'll find a lot of them to be nitpicks that reflect lessons learned from maintaining a lot of providers for a long time, and how little things like error messages can be structured to make future maintenance easier. So just because I'm giving you a laundry list of things to address here doesn't mean your provider is bad or your code is sub-par, I'm just trying to pass on the things we've learned the hard way so you can take advantage of them.

Here's what I found:

• [x] replace context.TODO() with context.Background()
• [x] data_source_linode_compute_ipv6_pool has a d.Get("") for reqPool, which makes no sense. Should there be a required field on the resource?
• [x] data_source_linode_compute_ipv6_pool should return the pool that wasn't found as part of the error message, to add context.
• [x] data_source_linode_compute_ipv6_pool has no tests.
• [x] data_source_linode_compute_ipv6_range reads range, but range will never be set, because it's Computed.
• [x] data_source_linode_compute_ipv6_range should return the range that wasn't found as part of the error message, to add context.
• [x] data_source_linode_compute_ipv6_range has no tests.
• [x] provider.go, line 70: return nil, fmt.Errorf("Failed to connect to the Linode API because %s", err) We tend to favor error messages like Error connecting to Linode API: %s, which is more flexible with a wider variety of error messages.
• [x] WaitTimeout is generically named for something that applies solely to instance status changes.
• [x] Empty init function in resource_linode_instance.go should be removed.
• [x] Why cache the kernelList, regionList, and typeList information in global mutable variables?
• [x] Caches have no concurrency controls and aren't safe for parallel access.
• [x] What made you choose to use InputDefault so heavily? It's not wrong, it's just unusual, and I'm interested in the thought process there.
• [x] If linode_instance's image is optional, and unset, does the API set a default value? If so, it should probably be Computed.
• [x] Re: linode_instance's name field, Optional is required when Removed is set because it needs to be a field that the user can set (or Removed is meaningless) but can't be required (or the user gets an error whether they set it or not).
• [x] We generally like Removed messages to be more along the lines of Please use $REPLACEMENT instead. See $URL for more information. When using Removed. The URL isn't required, but is a nice inclusion.
• [x] If linode_instance tags 1) can't be repeated 2) don't have a specific order and 3) aren't guaranteed to come back in the same order, I'd recommend making them a TypeSet.
• [x] If linode_instance's plan_storage can't have user input, there's not much point in marking it Removed, honestly.
• [x] If linode_instance's private_networking isn't set, does the server set a default? If so, Computed should be set.
• [x] Same suggestion for linode_instance's ssh_key field as for tags.
• [x] I'd highly recommend against using PromoteSingle.
• [x] root_password should have Sensitive: true.
• [x] When removing a resource from state because it wasn't found in the API, log a [WARN] removing %q from state because it no longer exists.
• [x] In resourceLinodeInstanceExists if there's an error retrieving the instance, it erroneously reports that there was an error parsing the ID.
• [ ] It'll save time and headache later to just move the ID parsing to a helper function
• [x] Exists functions should not modify the schema.ResourceData, including setting the Id.
• [x] Remove commented out code.
• [x] All fields available from the API should be set in state when reading. I noticed linode_instance doesn't set, e.g., tags or ssh_key in state when reading; if either is returned from the API, it should be set in state. State can be accurately thought of as a local copy of what the API has, and should be updated and maintained as such.
• [x] Why does linode_instance return if there isn't exactly one config? That has led to subtle bugs before, where things don't get set, because someone accidentally writes the code to set them after that. The code that uses the config should instead happen inside an if block, to avoid returning out of the function early and missing code that could have and should have run.
• [x] There shouldn't be any need for boolToString.
• [x] We may have talked about disks, and if that's the case, please forgive me, but why does linode_instance store only the cumulative size of all the disks instead of storing each disk in a sub-block, even if it's read-only?
• [x] Use timeouts instead of hardcoding them.
• [x] Why does the swap partition get hardcoded and special cased in linode_instance, instead of having the linodego.InstanceDiskCreateOptions struct represented as a sub-block in linode_instance?
• [x] Same with storage?
• [x] Same with linodego.InstanceConfigCreateOptions—why is so much of that hardcoded instead of obtained from user input?
• [x] Read should be called at the end of Update.
• [ ] I'm a little :/ about rebooting the instance without showing it in the plan, but I don't have a better suggestion for you. Consider making the things that would require a reboot ForceNew instead, so we can clearly indicate to users what will happen as part of an apply, especially when downtime may be a result?
• [ ] I believe HasChange can return true even if it's going from unset to false, or false to unset, making the private_networking update on linode_instance aggressive. Maybe use GetChange and compare explicitly from true => false to only error when absolutely necessary? Also, CustomizeDiff can be used to make changing this force a new resource, instead of throwing an error, which is a preferable UX, I think?
• [x] When testing, acctest.RandomWithPrefix can be used instead of fmt.Sprintf.
• [ ] When testing, make sure to check the expected values of all the fields in state, to ensure they were set correctly.

Please let me know if any of this doesn't make sense or you'd like more information on anything.

Best, Paddy

[displague]

Rather than copy your TODO list, I'm going to check off the things I believe have been covered, currently included in master.

[cgriggs01]

Hi @displague,

Thanks for all your hard work on this provider so far, I wanted to get an idea of then this provider is going to be ready for official release. Do you have an eta on when all the issues Paddy raised will be addressed?

Looking forward to the release. Best, Chris

[displague]

Hi @cgriggs01,

I've been trucking through the issues raised by @paddycarver above and incorporated linode_instance resource changes discussed elsewhere with @paddycarver and @paultyng (both of which have been very helpful at every step).

These changes are being applied in #19 with a goal of supporting and documenting the schema described in the example in that PR description.

In terms of linode_instance resource CRUD work, I have the CR_D done and now need to refactor Update behaviors to match. I believe I will have that work merged within a week.

I've also added support for Volumes, NodeBalancers, and DNS, so I'm hopeful that all of these can be considered for Review 2.

[displague]

All of the changes I could act on based on this feedback have been merged into master.

Thanks, @paddycarver

[paddycarver]

So I have a few quick notes after reading through the provider. I think after this, you're all set. Feel free to ping me when you've addressed these concerns, and I'll come sign off. These should be all minor things.

• I noticed you had a bunch of descriptions that specified valid input. For example, all the _sec fields on domain and domain_record, client_conn_throttle on nodebalancer, check_timeout and check_attempts on nodebalancer_config, and so on. You can specify a ValidateFunc on the schema.Schema for these, which will allow Terraform to raise errors at plan time. The validation package has some built-in: validation.IntBetween can be used when you want to be sure an int is between two values, and validation.StringInSlice can be used when you want to make sure that a string matches one of a few values. We don't have an IntInSlice yet, but it should be pretty straightforward to write one.
• there were a couple places where you marked a Set or List as Computed (meaning not user-settable) but set the fields within it to Required. I'm thinking of user_defined_fields in stackscript, transfer on nodebalancer, and backups on instance. The parents should either be set to Optional or Required (depending on which applies), or the children should be set to Computed with Optional and Required unset.
• ComputedWith doesn't really work and shouldn't be used.
• You have a few tests that use ImportState, but don't use ImportStateVerify. To test import properly, you should use ImportStateVerify there, too.
• nodebalancer_node and nodebalancer_config should have import tests on them, ideally.
• You have a few fields that use ConflictsWith. When field A conflicts with field B, field A's ConflictsWith needs to include B, and field B's ConflictsWith needs to include A. They've got to be included in each other's ConflictsWith, basically. In a few places, you only have them in one, not both.

That's it! You're at the home stretch. Thank you for being so superhumanly patient.

[displague]

Didn't intend to close the Issue, but #29 did cover the previous list of changes.

[paddycarver]

I've reviewed everything up to ae9e50a, and you went above and beyond in response to the last round of review. This looks good to me. I'm happy with the technical quality of it. 👍 Great work!

[displague]

Closing -- See Issue #36