Provider Review

[bill-rich]

Hello!

My name is Bill, I'm a member of the Terraform team @ HashiCorp.

I’ve taken a look at the provider here and would like to say great work so far! I do have feedback outlined below that I’d like to see addressed before we move on to the next steps. I’m opening this issues as a sort of checklist for tracking remaining items and discussion. The review was done on the git commit 03ec8e3 .

• Is the “resell” in the name needed since it is on all resources? It is fine if it is, but if it can be cut, it could make the names a little simpler.
• TypeSet may be better for servers, all_quotas, resource_quotas, quotas, and theme. With TypeList, the order matters. If the order could change during reads, it could cause unwanted diffs with TypeList (Use TypeSet instead of TypeList for projects, floatingips and licenses attributes #48).
• Required attributes should be grouped at the top of the resource followed by the optional attributes (Reorder required attributes #54).
• The error on setting non-scalars should be checked (servers, theme, quotas, all_quotas) (Check errors on setting non-scalars #52).
• Adding in a check for if resources don’t exist during read and unsetting the Id will allow Terraform to remove resources from the state that were deleted outside of Terraform and not cause errors (Unset IDs on resources read errors #53).
• In the documentation, you can leave arguments off the attributes section (Small documentation fixes #50).
• In license docs, I think “A type of the license” should be “The type of license” (Small documentation fixes #50).
• In project docs, “3d-level” should be “3rd-level” (Small documentation fixes #50).

Thank you for taking the time to work through these questions and comments with me!

Cheers,
Bill

[ozerovandrei]

@bill-rich Greetings.

I agree with all points, will fix them shortly.

[ozerovandrei]

@bill-rich I created PRs to address all points except removing “resell” prefix from resource names.
I’m going to fix it after merging all other commits.

You can check all open PRs, they’re waiting to be reviewed by you.

[ozerovandrei]

@bill-rich after discussing removal of the resell_ prefix with my team we came to the conclusion that we can't remove it.

We have two-level API hierarchy in our SelVPC API and Resell is used by the top-level user (reseller). Other APIs can be used by regular users but they can have similar resources. And those resources will have less attributes.
For example - the reseller can select broader parameters for creating public subnets, when the regular users can only create subnets of the fixed prefix length, etc.

The go-selvpcclient library that is used by terraform-provider-selvpc also uses this schema, so for now they have similar object names and it will be great if it stays that way.

I hope it's not a problem, and we can release the provider without renaming the resources. Let me know what you think.

[bill-rich]

@ozerovandrei That is totally fine to not remove the resell_ prefix. I just wanted to ask to make sure it was necessary. As long as it serves a purpose and is useful for differentiating different resources, it makes perfect sense to keep it.

Thank you for addressing all the questions and issues in the review. The work you've done looks great!

[ozerovandrei]

@bill-rich thank you for review!

I'm closing this issue because we've discussed the release of the provider with Chris.