Terraform Provider Development Program - Initial Review

cgriggs01 commented 5 years ago

Hey Matt,

My name is Chris, I'm a member of the Partner 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 items and discussion.

[ ] The documentation for this provider will be published onto Terraform.io once released, which means the you docs/ directory should be moved into a new website/ directory that must contain a Ruby layout file that will act as the sidebar menu in the Terraform.io documentation for this provider. The TLS provider has a good example of this.
[ ] All of our Terraform Providers use a MPL2.0 license, can you update that from the MIT license you currently have?

[ ] The makefile must be consistent will all other provider GNUmakefile as it's used in a few different CI processes, you will just need to update the PKG_NAME on line 4 to octopusdeploy
[ ] Once the provider is released, there will be a Travis check run on each PR. You'll need to add this .travis.yaml file for that test.
[ ] There are four scripts that are required for the testing a deployment of the terraform provider, here's a good example of the scripts/

As well I want to include this list of Terraform provider code expectations/conventions that were define by the Terraform engineering team.

[ ] resource filename matches convention resource_providername_resourcename.go
[ ] data source filename matches convention data_source_providername_resourcename.go
[ ] we always let d.Set() to dereference pointers safely to avoid crashes, i.e. there's nothing like d.Set(*variable)
[ ] flatteners and expanders are in structures.go + tests in structures_test.go
[ ] any validation functions longer than a few lines are in validators.go + tests in validators_test.go
[ ] d.Set() is called in any C/R/U/D for all available fields unless they're Computed-only (i.e. there are no orphans)
[ ] all of d.Set(), d.Get(), d.GetOk(), d.GetChange() etc. use real field names (i.e. no typos)
[ ] d.Set() is always error-checked if it’s set, list or map
[ ] d.Set() is never error-checked if it's primitive data type (string, int, float, bool)
[ ] Resource uses either Read or Exists function to remove resource from state if it's gone
[ ] d.Partial() & d.SetPartial() are used any time there's more than 1 API call used to create or update a resource.
[ ] schema.ForceNew is used for any field that is not updatable
[ ] all field names are lowercase_underscore
[ ] where possible data source fields match resource (e.g. when referencing aws_instance user shouldn't need to change anything - just add data. prefix)
[ ] Constants are used if there are available in the upstream SDK (e.g. for status codes)
[ ] [INFO] log message at least 2 per each C/R/U/D - e.g. creating (input), created (output)
[ ] The provider code does not panic, always returns errors

If you have any question about the feedback let me know.

Best, Chris

cgriggs01 commented 5 years ago

Along with the feedback, I'm getting a number or error when trying to build the provider. FYI, I'm running go1.11 on my local machine.

Any suggestion on getting the provider to build?

$ make test
==> Checking that code complies with gofmt requirements...
go test -i $(go list ./... |grep -v 'vendor') || exit 1
# github.com/terraform-providers/terraform-provider-octopusdeploy/octopusdeploy
octopusdeploy/deployment_action.go:93:3: unknown field 'IsRequired' in struct literal of type octopusdeploy.DeploymentAction
octopusdeploy/deployment_action.go:108:9: action.WorkerPoolId undefined (type octopusdeploy.DeploymentAction has no field or method WorkerPoolId)
octopusdeploy/deployment_action.go:115:10: action.Packages undefined (type octopusdeploy.DeploymentAction has no field or method Packages)
octopusdeploy/deployment_action.go:122:10: action.Packages undefined (type octopusdeploy.DeploymentAction has no field or method Packages)
octopusdeploy/deployment_step.go:32:28: undefined: octopusdeploy.DeploymentStepPackageRequirement_LetOctopusDecide
octopusdeploy/deployment_step.go:34:16: undefined: octopusdeploy.DeploymentStepPackageRequirement_LetOctopusDecide
octopusdeploy/deployment_step.go:35:16: undefined: octopusdeploy.DeploymentStepPackageRequirement_BeforePackageAcquisition
octopusdeploy/deployment_step.go:36:16: undefined: octopusdeploy.DeploymentStepPackageRequirement_AfterPackageAcquisition
octopusdeploy/deployment_step.go:43:28: undefined: octopusdeploy.DeploymentStepCondition_Success
octopusdeploy/package_reference.go:56:66: undefined: octopusdeploy.PackageReference
octopusdeploy/deployment_step.go:43:28: too many errors
make: *** [test] Error 1

Thank you for your help!

MattHodge commented 5 years ago

Hi @cgriggs01 , the provider should build successfully now!

Thanks for the big to do list 👍

cgriggs01 commented 5 years ago

Thanks @MattHodge,

I'll rebase and build again.

Let me know when you've addressed the other items.

Best, Chris

cgriggs01 commented 5 years ago

Hey Matt,

I'm still getting the same error during compilation. Any thoughts?

[ ] Also can you vendor checkin all the necessary dependancies into the repository?

MattHodge / terraform-provider-octopusdeploy

Terraform Provider Development Program - Initial Review #25