terraform-provider-vultr-community

terraform-provider-vultr-community copied to clipboard

Hey Team, 👋 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 review was done on the develop branch at git commit 01aecfc

• [x] All the attributes should be read and set in READ so that differences between the configuration and the actual state can be caught. Any non-scalar values (TypeList, TypeSet, TypeMap) should have errors checked when setting. The first instance of this is the v6_networks attribute in the BearMetalServer resource. Check out this plan attribute in the Azure provider for reference. ](https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/azurerm/resource_arm_virtual_machine.go#L743)

• [x] All official Terraform provider use a Mozilla Public License 2.0](https://www.mozilla.org/en-US/MPL/2.0/) within each repo, can you add this LICENSE](https://github.com/terraform-providers/terraform-provider-azurerm/blob/master/LICENSE) file to this repository.

• [x] There are three Makefile targets that should be added for the providers website documentation. website, website-lint, and website-test. They can be found in any of the provider GNUmakefile](https://github.com/terraform-providers/terraform-provider-azurerm)

• [x] When running the Go acceptance testing there was just one test that continuously failing. Any thoughts on what might be causing it?

=== RUN   TestAccVultrBackup
--- FAIL: TestAccVultrBackup (1.17s)
    testing.go:568: Step 0 error: errors during refresh:

        Error: your search returned too many results : 2. Please refine your search to be more specific

          on /var/folders/bf/6n0whcjn2x17rp19q0yc2vfc0000gn/T/tf-test568318980/main.tf line 2:
          (source code not available)

Hey @cgriggs01 thanks for for looking into our provider and giving us some feedback!

I'll start working on the recommend changes you laid out and get back to you once these are all done.

As for that failing acceptance test maybe you could give us your thoughts on this.

The backups these run on a cron that you define (During instance creation) They also take a while for them to finish and get displayed on your account. Now this isn't a big deal but the other caveat with backups is that they have a limited time span before they get automatically deleted.

Because of this there is a period where we will have two backups of the same name living together. This is why the test keeps failing. We haven't found a great way to really test backups other then having the cron run the backups once a month and then cleaning up the duplicated backup that will expire in X days. Also since backups are only only available as a GET and defined when you create a instance it proved to be a bit tricky.

Thoughts on this?

Hey!

Thanks for addressing these items and providing context around the failed acceptance test. Seem like there would have to be additional API functionality to allow TF to DELETE a backup.

One final review and we can begin the migration and release process.

@cgriggs01

Currently there is no API functionality to DELETE a backup. This can only be done via the UI. We will look into adding API functionality to allow for TF to delete a backup.

@cgriggs01

I made a change to the datasource for backups. After diving into this a bit more the original implementation for backups was the wrong approach.

Since a user can have a backup be taken everyday for X servers a user can have hundreds of given backups. To accommodate this we had changed the datasource to return back a list of backups instead of a single one.

This will address the issue of the original implementation only ever returning one even though it is very possible that you can have X amount all with the same name or same timestamp.

Let me know what your thoughts are on this approach. Also the PR is #108

@ddymko

Thanks for all your work so far. Everything looks great, just a few more items to complete the website docs.

• [x] In the documentation arguments should be tagged "Required" argument if they are required.

• [x] You should organize the objects in your resource and data source schema by type, Required arguments should be grouped at the top, followed by Optional arguments, then the Computed attributes

After these, we should be ready to migrate and release the provider.

@cgriggs01

Thanks for the feedback. I made all of the requested changes and merged them into the master branch.

I was wondering if I should cut a new release version with these changes or once the provider is migrated there is a different version that you would want to release it as?

@ddymko

Thanks for getting those addressed. These changes should be included in the initial release of the provider!

Let me know when your ready and I'll begin the migration and release process.

Best, Chris

@cgriggs01

Awesome! We would like to release as soon as we can.

I do have a few open questions

• How long does the whole process take before we would be posted as an official provider on terraform.io?
• Is there a blog post or any announcement of Vultr becoming an official provider and when would that get released?

Reason I ask is we would like to line up our own blog post article about becoming an official provider and have our blog post point over to the terraform.io docs for Vultr

@ddymko

The release should be complete by the end of today. If we don't run into any issues. Once the release is complete, then the documentation will be live on terraform.io.

We have a monthly integration newsletter that highlights the newly certified Terraform providers. Beyond that, I'll have to put you in contact with our partner marketing team to discuss further.

Once this repo has been moved I can add any maintainers that will need "write" access to the new repo for committing code and merging PR. Can you provider a list of (name, email, Github username) for everyone that should be a maintainer?

@cgriggs01

Sounds great!

If it is alright with you would we be able to do the migration on Thursday? (I am away today/tomorrow).

As for maintainers it would be

• David Dymko [email protected] https://github.com/ddymko
• Fady Farid [email protected] https://github.com/afady

Yes, that is fine with me, we'll plan to migrate and release on Thursday.

Talk to you then!

@cgriggs01

Let me know when you are ready to start the migration and what is required!!

Ready to go!

Should be straight forward, I'll move the provider code over to the new repo and then kick of the first build which will also publish the website docs. Just have to make a few internal PRs to set everything up.

@cgriggs01

Do you need me to to transfer this repo over to https://github.com/terraform-providers??

No, we just do a clone and push up to the new location at https://github.com/terraform-providers/terraform-provider-vultr

ah - gotcha sounds good!

Would you like to release the 1.0.4 version or bump up the version number?

@cgriggs01 We can bump it version v1.0.4 unless you think it would be better to do a new version since it became an official provider??

Since you already have changes and release docs for your previous v1.0.4 release and we have a different format for the Changelog, I'd suggest we do the initial release today at v1.0.5. If its ok with you all update the changelog and publish the release

I was just going to suggest we do a version bump since we have changes...so an initial release of v1.0.5 would be fine.

Should I rewrite the changelog now to match the format that is used by you?

Great, thanks.

You can reformat to the style if you'd like. Its only required moving forward because their is a bot used during the release that modifies the Changelog and publishes release tag to the repo.

Ah ok - so moving forward I'll make sure we just use the changelog format you guys have in place!