orange-cloudfoundry / terraform-provider-cloudfoundry

A terraform provider to manage a Cloud Foundry instance.
Apache License 2.0
31 stars 8 forks source link

Refactor resource loading #12

Closed dcarley closed 7 years ago

dcarley commented 7 years ago

I'm interested in using this project and writing new resources. However I found it harder to comprehend the layout of the code compared to other Terraform providers (core and non-core) that I've worked on in the past.

I'd to run some ideas by you that I think will make the code easier to understand. I've broken each change down into an individual commit and described in the commit message what I'm trying to solve. Could have a look through them and let me know your thoughts?

ArthurHlt commented 7 years ago

Hi @dcarley , thank you for the work. I like ideas you had except about removing a structure and set a bunch of functions in this format resource<ResourceName><CrudOperation>.

You actually doing the same thing as using a structure without use it. Struct and function attachment has been designed for this kind of usage. This is also better after for testing (which I really need to finish :) ).

For me it's a bit tricky, that's maybe done in others provider but I don't think it's a good way to code in go more generally. Of course, you can discuss about it and let me know why it should be like this and I'm probably too influenced by OOP languages. For me, order with the a structure is better than simply do it by functions name (and remind each time the function name instead of remain just Read, Create ...)

dcarley commented 7 years ago

I'm fine with omitting that commit. Some people feel very strongly about "idiomatic" non-OOP code in Go, but that alone isn't a good reason for me to tell you to change something. I believe the other commits are more important because they tangibly make the codebase more readable.

Something else we could do, which I think would help, is add a godoc string to describe the purpose of CfResource and re-order the public methods in each resource to be at the top of the file. What do you think?

One aspect that I forgot to mention is that it's not currently possible to introduce a resource that doesn't implement the Exists() method. Then again, I'm not sure whether it's strictly necessary for the other resources that implement it, because I believe Terraform uses Read() to determine whether it needs to call Create() or Update(). I guess this is where testing schema.Resource (which might only be possible with acceptance tests) would help.

ArthurHlt commented 7 years ago

about the Exists() method i feel the same as you do. It was actually in terraform and there is no doc about this, i just saw few provider use it but i can't figure out why. I'm good to remove it :)

Just why travis failed ? i will merge it when it pass but can't see why error 2, if you can take a look

dcarley commented 7 years ago

No idea why it failed. I've rebased and now it passes.

about the Exists() method i feel the same as you do. It was actually in terraform and there is no doc about this, i just saw few provider use it but i can't figure out why. I'm good to remove it :)

I'll figure that out in a different PR, if necessary.

ArthurHlt commented 7 years ago

Thank you @dcarley