{"action":"closed","issue":{"url":"https://api.github.com/repos/displague/terraform-provider-linode/issues/16","repository_url":"https://api.github.com/repos/displague/terraform-provider-linode","labels_url":"https://api.github.com/repos/displague/terraform-provider-linode/issues/16/labels{/name}","comments_url":"https://api.github.com/repos/displague/terraform-provider-linode/issues/16/comments","events_url":"https://api.github.com/repos/displague/terraform-provider-linode/issues/16/events","html_url":"https://github.com/displague/terraform-provider-linode/issues/16","id":346915514,"node_id":"MDU6SXNzdWUzNDY5MTU1MTQ=","number":16,"title":"Terraform Provider Development Program, Review 1","user":{"login":"paddycarver","id":59249,"node_id":"MDQ6VXNlcjU5MjQ5","avatar_url":"https://avatars0.githubusercontent.com/u/59249?v=4","gravatar_id":"","url":"https://api.github.com/users/paddycarver","html_url":"https://github.com/paddycarver","followers_url":"https://api.github.com/users/paddycarver/followers","following_url":"https://api.github.com/users/paddycarver/following{/other_user}","gists_url":"https://api.github.com/users/paddycarver/gists{/gist_id}","starred_url":"https://api.github.com/users/paddycarver/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/paddycarver/subscriptions","organizations_url":"https://api.github.com/users/paddycarver/orgs","repos_url":"https://api.github.com/users/paddycarver/repos","events_url":"https://api.github.com/users/paddycarver/events{/privacy}","received_events_url":"https://api.github.com/users/paddycarver/received_events","type":"User","site_admin":false},"labels":[],"state":"closed","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":5,"created_at":"2018-08-02T08:43:27Z","updated_at":"2018-10-12T19:20:56Z","closed_at":"2018-10-12T19:20:56Z","author_association":"NONE","body":"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.\r\n\r\nI 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.\r\n\r\nHere's what I found:\r\n\r\n- [x] replace `context.TODO()` with `context.Background()`\r\n- [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?\r\n- [x] data_source_linode_compute_ipv6_pool should return the pool that wasn't found as part of the error message, to add context.\r\n- [x] data_source_linode_compute_ipv6_pool has no tests.\r\n- [x] data_source_linode_compute_ipv6_range reads `range`, but `range` will never be set, because it's `Computed`.\r\n- [x] data_source_linode_compute_ipv6_range should return the range that wasn't found as part of the error message, to add context.\r\n- [x] data_source_linode_compute_ipv6_range has no tests.\r\n- [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.\r\n- [x] `WaitTimeout` is generically named for something that applies solely to instance status changes.\r\n- [x] Empty `init` function in `resource_linode_instance.go` should be removed.\r\n- [x] Why cache the `kernelList`, `regionList`, and `typeList` information in global mutable variables?\r\n- [x] Caches have no concurrency controls and aren't safe for parallel access.\r\n- [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.\r\n- [x] If `linode_instance`'s `image` is optional, and unset, does the API set a default value? If so, it should probably be `Computed`.\r\n- [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).\r\n- [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.\r\n- [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`.\r\n- [x] If `linode_instance`'s `plan_storage` can't have user input, there's not much point in marking it Removed, honestly.\r\n- [x] If `linode_instance`'s `private_networking` isn't set, does the server set a default? If so, `Computed` should be set.\r\n- [x] Same suggestion for `linode_instance`'s `ssh_key` field as for `tags`.\r\n- [x] I'd highly recommend against using `PromoteSingle`.\r\n- [x] `root_password` should have `Sensitive: true`.\r\n- [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`.\r\n- [x] In `resourceLinodeInstanceExists` if there's an error retrieving the instance, it erroneously reports that there was an error parsing the ID.\r\n- [ ] It'll save time and headache later to just move the ID parsing to a helper function\r\n- [x] `Exists` functions should not modify the `schema.ResourceData`, including setting the Id.\r\n- [x] Remove commented out code.\r\n- [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.\r\n- [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.\r\n- [x] There shouldn't be any need for `boolToString`.\r\n- [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?\r\n- [x] Use [timeouts](https://godoc.org/github.com/hashicorp/terraform/helper/schema#Resource.Timeouts) instead of hardcoding them.\r\n- [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`?\r\n- [x] Same with `storage`?\r\n- [x] Same with `linodego.InstanceConfigCreateOptions`—why is so much of that hardcoded instead of obtained from user input?\r\n- [x] `Read` should be called at the end of `Update`.\r\n- [ ] 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?\r\n- [ ] 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`](https://godoc.org/github.com/hashicorp/terraform/helper/schema#Resource.CustomizeDiff) can be used to make changing this force a new resource, instead of throwing an error, which is a preferable UX, I think?\r\n- [x] When testing, `acctest.RandomWithPrefix` can be used instead of `fmt.Sprintf`.\r\n- [ ] When testing, make sure to check the expected values of all the fields in state, to ensure they were set correctly.\r\n\r\nPlease let me know if any of this doesn't make sense or you'd like more information on anything.\r\n\r\nBest,\r\nPaddy"}}
{"actor":{"display_login":"displague"}}