Provider Review

[mbfrahry]

Hello!

I'm here to provide a review after discussing the provider a couple of weeks ago. Since I wrote this provider a couple of years ago, reviewing it was quite odd and seeing some of the missteps I took is a treat! There is quite a bit to unpack in this review but overall the provider is in a good state. I've ranked this list in order of importance with the most important issues being at the top. Feel free to comment and ask questions and I'll do my best to answer them.

• [ ] OneView returns an unordered array of various attributes and that’s causing issues when we use TypeList since a TypeList is ordered in Terraform. So if the Array comes back in a different order than what Terraform expects, Terraform will do an unnecessary update. When I wrote this resource, I got around this issue by reordering the list we got back from the api to match what Terraform was expecting but instead we can just swap the TypeList to a TypeSet and get rid of these bits of code. A TypeList cares about order while a TypeSet does not. Here are a couple places that could be updated to remove this logic.https://github.com/HewlettPackard/terraform-provider-oneview/blob/master/oneview/resource_logical_interconnect_group.go#L826 https://github.com/HewlettPackard/terraform-provider-oneview/blob/master/oneview/resource_network_set.go#L32

• [ ] Power state is a tricky topic in Terraform because we believe that the infrastructure Terraform manages should be immutable and changing the power state goes against that. We should consider removing it and leaving it up to another tool to manage it if needed.

• [ ] The docs folder will need some restructuring to work with how we build our website. It should look like

  website/
  oneview.erb (this is the sidebar. You can use another provider’s erb as a reference)
  docs/
      index.html.markdown
      r/
      d/ (if you add datasources)

• [ ] The tests should be expanded on with further checks and additional update steps where applicable.

• [ ] The logic after creating a resource and setting the id is a little wonky. It should look like

  fcNetError := config.ovClient.CreateFCNetwork(fcNet)
  if fcNetError != nil {
  return fcNetError
  }
  d.SetId(d.Get("name").(string))

• [ ] Many computed attributes seem unneeded. We should look at computed attributes as something that a user can use in another resource and would be useful to check at times like if something generates it’s own ip or additional identification that could be used elsewhere. Things like modified, created, etag are probably not needed but you’ll have better judgement on how a user could be using OneView

• [ ] It looks like this isn’t being set. We should confirm that every attribute in the schema is being set in the read function.

• [ ] Any TypeList or TypeSet should have it’s own corresponding expand and flatten functions to increase readability (i.e. https://github.com/HewlettPackard/terraform-provider-oneview/blob/master/oneview/resource_logical_interconnect_group.go)

• [ ] Constants would be preferred in cases where a string is being used. (ie. https://github.com/HewlettPackard/terraform-provider-oneview/blob/master/oneview/resource_logical_interconnect_group.go#L846)

• [ ] Just about any attribute that is Optional without a Default should be retrieved though d.GetOk. This prevents an empty string from being set and being misinterpreted by the api. If the api expects an empty string than d.Get is just fine.

• [ ] Some comments are referring to the wrong resource

• [ ] I believe we can clean up some of the other files in the repo as well. Do you have any issue with removing HACKING.md, Dockerfile, and CONTRIBUTING.md

• [ ] We'll want to swap the LICENSE over to MPL 2.0

• [ ] You can potentially look at adding import support so people who would like to use Terraform can quickly get it hooked into their existing infrastructure. It’s fairly simple if every attribute is being set properly. You can add

  Importer: &schema.ResourceImporter{
  State: schema.ImportStatePassthrough,
  },

  to the schema declaration and add

  {
  ResourceName:      resourceName,
  ImportState:       true,
  ImportStateVerify: true,
  },

  To a TestStep to confirm it’s hooked in properly.

• [ ] Datasources are anothe useful addition if you are looking to read attributes but not necessarily modify the resource it’s attached to. You can take one of your resources and strip the create, delete, and update methods from them to create datasource. Other providers would be a good reference on how it looks.

[mbfrahry]

A few more notes from our discussion earlier

• [ ] Here is a fairly simple datasource. Note that the read function is the only method necessary as you are not modifying any attributes with a datasource

• [ ] Basic Expand and Flatten(https://github.com/terraform-providers/terraform-provider-opc/blob/d3333cf98e56dbc95997bca1956826df680e8ea0/opc/resource_lbaas_policy.go#L709) functions are here. This is just taking what is already existing inside of the oneview provider and moving it for readability

• [ ] We also need to update the readme for the latest version of go(1.11.x) and terraform(0.11.x).

[nabhajit-ray]

We have added the following functionality:

1. Using typeset for un ordered attributes
2. Power state support for server hardware.
3. Import functionality for all resources

We have added the flatenning function usage, docs cleanup , extending testcases to out backlogs. I am closing this here.