Initial Provider Review

[paultyng]

Hello!

My name is Paul, 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 issue as a sort of checklist for tracking remaining items and discussion. The review was done on the git commit c8532f9.

• The provider should be renamed to something less ambiguous than cf (cloudfoundry?) It does seem like the internal package is called cloudfoundry. Maybe this is due to conflicts with the other cloudfoundry provider?
• What is generate-schema? how is it meant to be used? Is that solely for use in intellij? Is it possible to split that out of the repository?
• What is the .test_env folder for?
• For the acceptance testing AWS infrastructure, it would be great if it used Terraform instead of the scripts.
• For imports, I wouldn't re-alias names (ie. terraform as tf) unless you have a conflict, as it will throw off contributors who are used to unaliased ones.
• Review Go’s CodeReviewComments doc. A common thing I see in the code is named result parameters. While functional, naked returns and named results are not the most common go practices and are a little harder to follow.
• Please at least run goimports on the code (many editors can do this automatically, vim-go, vscode, etc.), optionally you could also use golint or even gometalinter to help find additional issues. Passing gometalinter is unnecessary for CI, but it’s a good tool when developing, below is a sampling of other issues I found with the tools.
• Missing error checks (found with errcheck), if these are meant to be explicit use _ and possibly comment as to why the error is discarded, and you should definitely log the error so it can be found in trace if desired:

cloudfoundry/provider_test.go:168:30:	sm.ForceDeleteServiceBroker(serviceBrokerID)
cloudfoundry/resource_cf_app.go:417:16:	am.DeleteApp(app.ID, true)
cloudfoundry/resource_cf_app.go:706:14:	am.DeleteApp(d.Id(), false)
cloudfoundry/resource_cf_route.go:139:18:	rm.DeleteRoute(route.ID)
cloudfoundry/resource_cf_user.go:200:11:	um.Delete(id)

• Some additional issues found with the linters:
• cloudfoundry/import_cf_service_instance_test.go:63:4 - for loop just has a return before looping?
• cloudfoundry/provider_test.go:380:3 - the err value is never actually returned
• cloudfoundry/resource_cf_app.go:462:3 - I imagine this is just still a work in progress?
• cloudfoundry/resource_cf_evg.go:86:2 - err is assigned but not returned?
• Spell check the website docs
• The vendor dir seems to include some unused stuff and the vendor.json seems to be out of sync with it, but I may be missing something, probably worth taking a pass at your dependencies (especially once goimports cleans up your imports) and ensure they are all necessary.
• Are the cfapi and repo packages SDKs? If so, we traditionally keep the SDK’s outside of the provider repos.
• It seems like the repo package is pulling a release locally, modifying it, and then it will be pushed up to Cloud Foundry as part of the resource_cf_app, is that understanding correct? You should switch to using the ioutil.Temp* methods instead of having the case where it uses the home directory. Also, please make sure to clean up any temp files or directories the provider creates during its execution. FWIW you may want to try to integrate this using an addition to the GitHub provider, HTTP, or Archive providers so you aren't left having to implement things like this for every possible storage location.
• For resource schema, if you have an optional attribute, typically (but not always) you would include a default.

I realize this is a lot but I think overall this is in good shape. Some of these issues may be due to misunderstandings of your use cases so feel free to let me know if so, or if there are any other things you'd like an eye on in this issue.

Thanks again!
Paul

[cgriggs01]

Assigning @mevansam to this issue.

[mevansam]

Hi @paultyng, thanks for providing the initial code review feedback. We prioritized the above list at our weekly meeting yesterday and plan to have them addressed within the next couple of weeks. We will reach back to you in this thread if we need any further clarifications.

[paultyng]

Feel free to drop a note here if you would like to discuss any of those items or drop me an email.

[gberche-orange]

Here is a rewording of the analysis from March 8th 2018 team weekly meeting

• The provider should be renamed to something less ambiguous than cf (cloudfoundry?) It does seem like the internal package is called cloudfoundry. Maybe this is due to conflicts with the other cloudfoundry provider?
  • => agreed. This is tracked into Rename d.service into d.service_instance  #81
• What is generate-schema? how is it meant to be used? Is that solely for use in intellij? Is it possible to split that out of the repository?
  • The generate-schema along with cf.json files are only needed for intellij support while the provider isn't yet official. Once it becomes official, a new release of https://github.com/VladRassokhin/intellij-hcl would support cloudfoundry provider. Prepared Cleanup intellij support #45 for this cleanup
• What is the .test_env folder for?
  • Currently holding config files travis-ci for our PR handling for sourcing credentials
  • => Mevan to import the credentials directly into travis and remove this encrypted file
• For the acceptance testing AWS infrastructure, it would be great if it used Terraform instead of the scripts.
  • pcfdev-up.sh and pcfdev-access.sh are being deprecated by @mevansam and replaced by https://github.com/mevansam/automation-pipelines/tree/dev/pcf
  • => tracked into Remove AWS scripts deprecated by mevansam/automation-pipelines #46
• Review Go’s CodeReviewComments doc. A common thing I see in the code is named result parameters. While functional, naked returns and named results are not the most common go practices and are a little harder to follow.
  • naked results refactoring is being tracked in Fix naked return code style #47
  • we'll open new issues if we see other coding idioms to fix, feel free to point them out if we miss some.
• For imports, I wouldn't re-alias names (ie. terraform as tf) unless you have a conflict, as it will throw off contributors who are used to unaliased ones.
• Please at least run goimports on the code (many editors can do this automatically, vim-go, vscode, etc.), optionally you could also use golint or even gometalinter to help find additional issues. Passing gometalinter is unnecessary for CI, but it’s a good tool when developing, below is a sampling of other issues I found with the tools.
  • @ArthurHlt to detail his proposal in at least run goimports on the code + prune unused vendor libs #48
• Missing error checks (found with errcheck), if these are meant to be explicit use _ and possibly comment as to why the error is discarded, and you should definitely log the error so it can be found in trace if desired:

cloudfoundry/provider_test.go:168:30:	sm.ForceDeleteServiceBroker(serviceBrokerID)
cloudfoundry/resource_cf_app.go:417:16:	am.DeleteApp(app.ID, true)
cloudfoundry/resource_cf_app.go:706:14:	am.DeleteApp(d.Id(), false)
cloudfoundry/resource_cf_route.go:139:18:	rm.DeleteRoute(route.ID)
cloudfoundry/resource_cf_user.go:200:11:	um.Delete(id)

• tracked in Missing error checks #49

[gberche-orange]

@paultyng we would like to prevent future regressions to suggestions you reported. Would should share URLs to some of the tools you used (e.g. linters) so that we can integrate them into our build ? We tried to see if terraform-provider-aws travis build used them but did not yet spotted them. Please add you comments into in #54

[gberche-orange]

@paultyng

Additional dispatching of issues as part of Tuesday April 4th team meeting

• Some additional issues found with the linters: tracked in Fix linter reported issues #53

• cloudfoundry/import_cf_service_instance_test.go:63:4 - for loop just has a return before looping?
• cloudfoundry/provider_test.go:380:3 - the err value is never actually returned
• cloudfoundry/resource_cf_app.go:462:3 - I imagine this is just still a work in progress?
• cloudfoundry/resource_cf_evg.go:86:2 - err is assigned but not returned?

• Spell check the website docs. Tracked in Spell check the site docs #55
• The vendor dir seems to include some unused stuff and the vendor.json seems to be out of sync with it, but I may be missing something, probably worth taking a pass at your dependencies (especially once goimports cleans up your imports) and ensure they are all necessary. Tracked in at least run goimports on the code + prune unused vendor libs #48
• Are the cfapi and repo packages SDKs? If so, we traditionally keep the SDK’s outside of the provider repos.
  • There is a fair amount of coordination with the CF CLI team to reach a sustainable api repo that we outlined into Move cfapi and repo package into a distinct repo #56. We suggest not to make this a prerequisite for the 1.0 release and move to next steps of making the provider join the hashicorp's official providers list.
• It seems like the repo package is pulling a release locally, modifying it, and then it will be pushed up to Cloud Foundry as part of the resource_cf_app, is that understanding correct? You should switch to using the ioutil.Temp* methods instead of having the case where it uses the home directory. Also, please make sure to clean up any temp files or directories the provider creates during its execution. FWIW you may want to try to integrate this using an addition to the GitHub provider, HTTP, or Archive providers so you aren't left having to implement things like this for every possible storage location.
  • The first part is now tracked into Rework cf_app local download to use ioutil.Temp & clean up temp files #57
  • The second part is tracked into Enable fetching of cf app binary bits by other TF providers  #58 and will be studied past 1.0 as this requires some coordination with other providers to support terraform-provider-cf use cases.
• For resource schema, if you have an optional attribute, typically (but not always) you would include a default. Now tracked as check which optional attributes should have a default #59

[gberche-orange]

@cgriggs01 @paultyng The team worked hard to get the 0.9.9 release out with the objective to reach a stable resource naming as to avoid breaking changes in the future, and having outstanding bugs fixed.

Most of the suggestions above have been addressed, with the exception of the list below which have been assigned to a later milestone:

• Move cfapi and repo package into a distinct repo #56 as we believe we should be leveraging an upcoming CF golang client library we're waiting for
• Enable fetching of cf app binary bits by other TF providers  #58 as this requires more work and inputs to Enable fetching of cf app binary bits by other TF providers  #58 (comment)

Note that there are few additional changes that we'd like to bring into a 1.0 version in the coming days. We believe that it should not affect too much the review process: #150 #147 #149 #125 #151

Besides, we'd like input on whether #126 is a prereq for an 1.0 official release.

We're eager to proceed with further the review of the provider, and moving forward with the Terraform Provider Development Program.

We'd like to learn how this could affect the team's travis-based CI workflow in the future (Travis-ci enforces that security credentials used to run acceptance tests can only be used on PRs of branches of the repos, and PR of from branches from forked repositories. All team members submitting PRs have therefore push access to the repo, although we have not yet completed the setup of the protected branches, probably dev and master).

[cgriggs01]

Hey Team, hope you all are doing well.

This review was unfortunately sidelined over the last month, but I'd like to get it back on track an accessible through terraform init.

Now that we've release Terraform 0.12, we'll need to make sure up update to the new 0.12 Terraform sdk.

Let me know if you have questions or are interested in meeting me for a short call to align next steps.

[ArthurHlt]

hi @cgriggs01 we actually working on the provider in best effort. I don't think we are matching all your requirements right now.

But at least we are compatible with terraform 0.12, we have move sdk inside the provider to its own repo and documentation is compatible with your new documentation format.

Thanks for your time

[candrews]

What's the latest news on this effort?

I'm quite excited for the prospect of having this provider distributed as part of terraform :)

[ArthurHlt]

@candrews do we follow your rules/requirements ?

Our actual state is:

1. We don't have anymore automatic integration test on github because our previous platform is not accessible because it was own by previous contributor/creator which is not contributing for now. We could create a full cloud foundry platform but we would need gcp or azure or aws to create it.
2. We are working on this project in best effort (so maybe sometimes response on issue and PR can be long)

The good part:

• technically we follow all your requirements (or close to, last point for using terraform-plugin-sdk will be added by Add id_bg attribute on app to handle resource link to blue-green app #243 ) - you can also see checked box above
• we are up-to-date with terraform

Let me know what is your thought about this, if we can distributed as part of terraform or not :)

[candrews]

I'm not actually with terraform - I'm just an interested user :-)

Thank you for the info - perhaps we can reach out to terraform and see if this is now includable?

[loafoe]

The provider is now published on the Terraform Registry. @cgriggs01 @paultyng would this provider qualify for a Verified flag?

[cgriggs01]

Hey Andy, Unfortunately at this time we can only onboard and verify providers from ISVs. We're still trying to figure out how to fit in open source organizations or foundations into our program. Hope this answers your question. Best, Chris

…

On Thu, Mar 31, 2022 at 7:13 AM Andy Lo-A-Foe ***@***.***> wrote: The provider is now published on the Terraform Registry. @cgriggs01 <https://github.com/cgriggs01> @paultyng <https://github.com/paultyng> would this provider qualify for a Verified flag? — Reply to this email directly, view it on GitHub <#38 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACU7UYSWZ7LHECARZ3XLVT3VCWXI5ANCNFSM4ETKEPDA> . You are receiving this because you were mentioned.Message ID: <cloudfoundry-community/terraform-provider-cloudfoundry/issues/38/1084645054 @github.com>

-- Chris Griggs(He/Him) Partner Alliances Manager, Terraform & Packer e ***@***.*** m +1 (530) 913-7497 <+15309137497> ------------------------------ <https://www.hashicorp.com/>

[loafoe]

@cgriggs01 thanks for the quick response! will close this ticket and open a new one once there is program for foundations.