Initial Provider review

[catsby]

Hello!

My name is Clint, 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 3cb3de9 on the stable branch.

• There is a good amount of use of the AWS SDK, I noticed for bucket/storage it references S3 often, and a credentials setup in config.go that use aws.Credentials, can you elaborate on that? Does the OpenTelekom Cloud replicate that API? 

• tags.go too uses a lot of AWS things and has mentions of ELBv2 tags and such. Please forgive my unfamiliarity, but is this code carried over from something and not used, or does the OpenTelekom Cloud support these things? I searched the code and didn’t find the method being invoked (setElbV2Tags, to provide a specific example)

• The S3 usage seems more inline, you’re reusing the AWS SDK S3 service I assume because the OpenTelekom bucket service is an S3 compatible API, is that correct? Other AWS inclusion should be reviewed 

• There are a number of methods and functions named TestAccAWS[…] that should be renamed or removed if not actually a test/part of this Provider

• there are some code blocks commented out in the schema of resource_opentelekomcloud_s3_bucket.go that should be removed if they are not supported. Other unsupported blocks that are from another Provider but not supported should be removed, if they exist in other resources. 

• The compute flavor v2 resource has a delete method that is unimplemented 

• In resource_opentelekomcloud_identity_user_v3.go: non-scalar attributes such as extra and multi_factor_auth_rules should have the error checked when calling d.Set in the read method, to ensure saving them to state was done correctly. This pattern should be adopted where appropriate across all resources. Here is an example from the AWS provider on checking the error: https://github.com/terraform-providers/terraform-provider-aws/blob/89bb35ee448d9cc3cfd4ac60b47b1f7316ddea91/aws/resource_aws_rds_cluster.go#L598-L608

• I see lb_pool_v2.go has a “CheckDeleted” method; that can be moved to simply implementing the “Exists” method at the schema level. An example can be found here: https://github.com/terraform-providers/terraform-provider-aws/blob/1c3a4c39ea1cd731d57efdf71b77110030289236/aws/resource_aws_kms_key.go#L22


Overall the Provider is mostly in good shape I would say. There are some AWS things in there that I think are vistigel and could be removed, and I’ve listed above some patterns and examples that should be applied to the the project as a whole and not just the specific resources I mention.

Cheers,
Clint

[gator1]

Thank you Clint, we made most of the changes as you requested and explain the rest.

There is a good amount of use of the AWS SDK, I noticed for bucket/storage it references S3 often, and a credentials setup in config.go that use aws.Credentials, can you elaborate on that? Does the OpenTelekom Cloud replicate that API? 


We only use AWS SDK for Swift S3 bucket support. I have cleaned unnecessary code use in utility code.

tags.go too uses a lot of AWS things and has mentions of ELBv2 tags and such. Please forgive my unfamiliarity, but is this code carried over from something and not used, or does the OpenTelekom Cloud support these things? I searched the code and didn’t find the method being invoked (setElbV2Tags, to provide a specific example)

OTC cloud doesn't support, and this has been fully removed.

The S3 usage seems more inline, you’re reusing the AWS SDK S3 service I assume because the OpenTelekom bucket service is an S3 compatible API, is that correct? Other AWS inclusion should be reviewed 


Correct. Other AWS inclusion should be gone now.

There are a number of methods and functions named TestAccAWS[…] that should be renamed or removed if not actually a test/part of this Provider

Fixed.

there are some code blocks commented out in the schema of resource_opentelekomcloud_s3_bucket.go that should be removed if they are not supported. Other unsupported blocks that are from another Provider but not supported should be removed, if they exist in other resources. 


Fixed.

The compute flavor v2 resource has a delete method that is unimplemented 

Was work in progress when copied from OpenStack. We don't use, so it is removed.
In resource_opentelekomcloud_identity_user_v3.go: non-scalar attributes such as extra and multi_factor_auth_rules should have the error checked when calling d.Set in the read method, to ensure saving them to state was done correctly. This pattern should be adopted where appropriate across all resources. Here is an example from the AWS provider on checking the error: https://github.com/terraform-providers/terraform-provider-aws/blob/89bb35ee448d9cc3cfd4ac60b47b1f7316ddea91/aws/resource_aws_rds_cluster.go#L598-L608

We can't test this, and clients don't have this privilege, so removed. As for the pattern, this is copied OpenStack code. It is a systematic problem, but I would prefer to have it fixed in OpenStack, and resolved when we modify our provider to use OpenStack provider as a library, rather than copying code, and then properly fixed in original OpenStack provider project.

I see lb_pool_v2.go has a “CheckDeleted” method; that can be moved to simply implementing the “Exists” method at the schema level. An example can be found here: https://github.com/terraform-providers/terraform-provider-aws/blob/1c3a4c39ea1cd731d57efdf71b77110030289236/aws/resource_aws_kms_key.go#L22

This is a systematic problem in our copied OpenStack code. I would prefer to have it fixed in OpenStack properly like the previous issue.

In the long term we intend to change the provider so that it uses Openstack provider as a lib and contribute to the community to support standard openstack features and only add support for our specific features here. Due to customers demand and time constrain we start this the current way.

[catsby]

Thank you for addressing the feedback, I'll take another look.

You mention "I would prefer to have it fixed in OpenStack" can you elaborate on your plans there?

Cheers,
Clint

[catsby]

I'm sorry, you actually answer my question right after that part, I just missed it 😄

I'll take another look at the code, thanks!

[catsby]

Thanks for updating the code here. I see most of the issues have been addressed and I've checked them off above.

I do have a question/comment about this:

As for the pattern, this is copied OpenStack code. It is a systematic problem, but I would prefer to have it fixed in OpenStack,

Unless this resource is a verbatim copy of the OpenStack resource, and it's planned to pull updates from the OpenStack Provider code, I'm afraid these patterns need to be addressed here. The same goes for my comment on the CheckDeleted method: unless you plan on pulling from the OpenStack provider code base regularly, and assuming those updated code changes apply cleanly, then this is a pattern that needs to be addressed here.

I fully understand that those bits of code were likely not written by anyone involved in the OpenTelekom Provider project, but by forking that project and going your own direction with it, it's expected that you own the code and address these issues. It's apparent now that the existing OpenStack provider has some anti-patterns or some items that should be improved as well 😄

The error checking on non-scalar attributes and possibly redundant CheckDelete methods are not blockers for proceeding and getting this provider into the Terraform-Providers organization though, so we'll begin to move forward

[catsby]

I've moved on to looking into the docs and found some broken links. Specifically the files https://github.com/gator1/terraform-provider-opentelekomcloud/blob/master/website/openstack.erb needs to be renamed opentelekomcloud.erb , and when reviewing the links therein I found several that were broken. Specifically the firewall_v1 resources, however all of them should be verified.

Thanks!

[gator1]

I have renamed openstack.erb to be opentelekomcloud.erb. I reviewed all the links in that file, and they should all be correct now.

We recently added some new resources to our master branch (a non-OpenStack load balancer). Should we wait until this review is done before merging that in?

[gator1]

I have fixed all non-scalar d.Set calls to check for errors, and will merge it in after it passes the acceptance test. As for the "CheckDeleted" method, I don't think I should remove it. Even if I implement the "Exists" method at the schema level, it is entirely possible that an asynchronous call has deleted the resource after the "Exists" call and before the "Read", so a simple check for a 404 return on this error and clearing out the ID here seems prudent and safe. In fact the code you referred me to essentially does the same thing. Is an implementation of the Exists method a new standards requirement? Even the AWS provider only implements it for 5 resources in the current code.

[catsby]

I have renamed openstack.erb to be opentelekomcloud.erb. I reviewed all the links in that file, and they should all be correct now.

👍

We recently added some new resources to our master branch (a non-OpenStack load balancer). Should we wait until this review is done before merging that in?

Probably a good idea to get the provider out there and then those new features can come after we release, if you don't object

I have fixed all non-scalar d.Set calls to check for errors

Thanks!

As for the "CheckDeleted" method, I don't think I should remove it

That's fine, it's not a blocker, not "wrong"

Is an implementation of the Exists method a new standards requirement?

Requirement, no. Good idea, probably. But you have the equivalent so it's fine, the important thing is that somewhere we're checking if it exists and acting appropriately (removing from state if so), so that users aren't simply blocked on resources that are no longer there.

Even the AWS provider only implements it for 5 resources in the current code.

This doesn't surprise me, I believe Exists came recently (past year or 2) and many of those resources predate it. Also, as I mention above, it's not a requirement.

I believe things look good here, We should proceed to the next phase which is actually merging into https://github.com/terraform-providers. I believe we need to edit the path/name in at least main.go for that.

If you don't object, I propose that once all the required items internally (at HashiCorp) are done for migrating to terraform-providers, I'll setup the repo and push this code as is. I'll then give yourself (and a few others, if you'd like) commit access, and you can then clone from the new location and make needed changes. How does that sound?

[gator1]

Probably a good idea to get the provider out there and then those new features can come after we
release, if you don't object

Agreed.

If you don't object, I propose that once all the required items internally (at HashiCorp) are done for
migrating to terraform-providers, I'll setup the repo and push this code as is. I'll then give yourself (and
a few others, if you'd like) commit access, and you can then clone from the new location and make
needed changes. How does that sound?

Sounds great. Do you have any idea on a time-frame for that?

[catsby]

I'm starting the process now! You should receive an invite to collaborate on the repository shortly.

[catsby]

Hey @gator1 sorry for the delay, you should have an invite for the new repo at https://github.com/terraform-providers/terraform-provider-opentelekomcloud