Add openstack_compute_flavor_v2 resource #83

mcanevet · 2017-08-22T14:02:28Z

jtopjian · 2017-08-23T01:59:40Z

@mcanevet This is a really good start.

I was able to get this working with the following fixes. Some of them are a little tricky, so they deserve some explanation:

The custom FlavorCreateOpts is only needed if you plan on having a Terraform parameter called value_specs. value_specs is where we have been allowing arbitrary key/value parameters into resources. This is usually required for vendor-specific OpenStack extensions. For example, let's say you had an OpenStack cloud managed by Big Vendor who requires tenant_id to be added to the request in order for the flavor to be created. You would then do:

resource "openstack_compute_flavor_v2" "my_flavor" {
  name = "my_flavor"
  ...
  value_specs {
    tenant_id = "abcd1234"
  }
}

Since the WIP flavor resource you created doesn't have value_specs, that was one cause of an error.

Unless you need value_specs, it is safe to not add it until someone specifically requests it. The reason for this is because for each custom type we make in types.go, we need to monitor possible upstream changes to Gophercloud.

createOpts needed to be passed as &createOpts. I suspect this is a bug in Gophercloud as no other Create function requires a pointer. I will open an issue at Gophercloud and ask about this.
The only place the id field needs to be set or read is with d.SetId() and d.Id().
Passing disk and swap as pointers is a two-step process rather than directly casting to *int.

If you'd like, I can submit a PR of my fixes to your branch, or you can copy them, or implement them yourself with any modifications you need.

One thing I recommend is also including parameters for is_public and ephemeral. It looks like ephemeral is not listed in the Gophercloud Flavor result type. I suspect this is also a bug.

With regard to the _test.go file I added, you can run this test by doing the following:

source ~/openrc
cd ~/go/src/github.com/terraform-providers/terraform-provider-openstack              
TF_LOG=DEBUG make testacc TEST=./openstack TESTARGS="-run=TestAccComputeV2Flavor_basic" 2>&1 | tee ~/openstack.log

I have a local bash function to quickly run tests:

ttest() {                                                                                   
  if [[ -n $1 ]]; then                                                                      
    pushd ~/go/src/github.com/terraform-providers/terraform-provider-openstack              
    TF_LOG=DEBUG make testacc TEST=./openstack TESTARGS="-run=$1" 2>&1 | tee ~/openstack.log
    popd                                                                                    
  fi                                                                                        
}                                                                                           

ttest TestAccComputeV2Flavor_basic

Let me know if you have any questions. :)

edit: Oh, nevermind about the &createOpts. It's just standard to declare createOpts like this. I just overlooked it :)

mcanevet · 2017-08-23T06:09:27Z

@jtopjian thanks a lot for your explanations.

Creation works with your fix. I'll see if I've time to look at gophercloud to add the update and delete support.

I first added parameters for is_public and ephemeral, but I noticed that they are not supported in the pinned version of gophercloud https://github.com/terraform-providers/terraform-provider-openstack/blob/v0.2.0/vendor/github.com/gophercloud/gophercloud/openstack/compute/v2/flavors/results.go#L34-L49 (they are in upstream) so I removed them from my PR for now.

fatmcgav · 2017-08-23T07:09:06Z

@mcanevet Feel free to update the vendor dep if necessary...

govendor update [package] should do the job...
Have a look at the commit history for example commit messages...

mcanevet · 2017-08-23T07:55:47Z

@fatmcgav I'm trying to update gophercloud with this command:

$ govendor fetch github.com/gophercloud/gophercloud@81a3d5b517a645123c0659518ac468bb14588619

But somehow it looks like it fails somewhere because I don't have the full diff I can see here : gophercloud/gophercloud@c5dfe1b...81a3d5b
I only have changes in vendor/github.com/gophercloud/gophercloud/STYLEGUIDE.md and vendor/vendor.json.
Any idea?

fatmcgav · 2017-08-23T08:07:33Z

@mcanevet I suspect govendor is just fetching that specific commit.

In the past, I've run govendor update github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/fwaas/routerinsertion/..., which updates the entire package.

So maybe try that on github.com/gophercloud/gophercloud/openstack/compute/v2/flavors/... and see what it does...

mcanevet · 2017-08-23T08:11:21Z

nothing happens...

fatmcgav · 2017-08-23T08:15:59Z

@mcanevet Hmm, did you revert your changes to vendor.json?

As I've just run govendor update github.com/gophercloud/gophercloud/openstack/compute/v2/flavors/... against master, and have the following changes reported:

# git status --short
 M vendor/github.com/gophercloud/gophercloud/openstack/compute/v2/flavors/requests.go
 M vendor/github.com/gophercloud/gophercloud/openstack/compute/v2/flavors/results.go
 M vendor/vendor.json

mcanevet · 2017-08-23T08:44:01Z

@fatmcgav after checkouting master it works on by branch...
I don't understand what happened, but whatever.
Thanks

fatmcgav · 2017-08-23T09:14:46Z

@mcanevet Heh, gotta love govendor ;)

fatmcgav · 2017-08-24T08:54:57Z

@mcanevet Are you intending to update Gophercloud to support Update/Delete?

Just wondering whether it's worth merging this one as is and then add support for Update & Delete operations once Gophercloud has been updated... Rather than leave it sitting open for a while :)

@jtopjian Thoughts?

fatmcgav · 2017-08-24T08:55:32Z

Also, I guess documentation needs to go on the 'TODO' list :)

mcanevet · 2017-08-24T09:42:58Z

@fatmcgav I'm not sure I'll have time to add support of update/delete to gophercloud.
I added documentation and squashed my commits.

fatmcgav · 2017-08-24T13:08:46Z

website/docs/r/compute_flavor_v2.html.markdown

+
+# openstack\_compute\_flavor_v2
+
+Manages a V2 flavor resource within OpenStack.


Is it worth adding a note that currently only 'Create' operation is supported?

@jtopjian Is there any prior-art for this?

fatmcgav · 2017-08-24T13:13:48Z

openstack/resource_openstack_compute_flavor_v2.go

+	createOpts := flavors.CreateOpts{
+		Name:       d.Get("name").(string),
+		RAM:        d.Get("ram").(int),
+		VCPUs:      d.Get("vcpus").(int),


For consistency, should RAM and VCPUs be assigned to variables as well?
Granted they'll just be passed straight through rather than pointered...

Appreciate that gophercloud expects them in different formats currently, but probably should standardise :)

@jtopjian Thoughts?

I think it's fine to leave this as-is. The pattern throughout the rest of the OpenStack resources is more or less:

passAsInt := d.Get("foo").(string) needsWorkDoneFirst := resourceComputeV2SomethingBar(d.Get("bar")) createOpts := something.CreateOpts{ Foo: &passAsInt, Bar: needsWorkDoneFirst, Baz: d.Get("baz").(string), }

Put another way, if you wanted to assign ram and vcpus as a variable as well, that opens the door to other contributors doing something like:

var1 := d.Get("var1").(string) var2 := d.Get("var2").(string) var3 := d.Get("var3").(string) createOpts := something.createOpts{ Var1: var1, Var2: var2, Var3: var3, }

While that's not bad and it technically works, I'd rather stick with something closer to how the rest of the resources look.

Fine by me... I guess ideally Gophercloud should be updated to use the same pattern throughout flavors.CreateOpts

The reason disk takes a pointer is because it's actually a tertiary type of setting: 0, non-zero, nil. :)

Ah, Ok... Lots of little nuances then :)

fatmcgav · 2017-08-24T13:15:26Z

openstack/resource_openstack_compute_flavor_v2.go

+			"ram": &schema.Schema{
+				Type:     schema.TypeInt,
+				Required: true,
+				ForceNew: false,


As Gophercloud doesn't currently support 'Update', is it worth setting these to ForceNew: true?

@jtopjian Thoughts?

@fatmcgav as Gophercloud does not support delete either, ForceNew: true will not help...

Ah, true... 😃

jtopjian · 2017-08-24T13:55:09Z

This will have to wait until Update and Delete are implemented in Gophercloud. It doesn't make sense to have a Terraform resource that only creates.

I can look at adding Update and Delete in the next week.

fatmcgav · 2017-08-24T15:49:24Z

@mcanevet I've opened a PR to add support for Delete operation to Gophercloud - gophercloud/gophercloud#501

Edit: So according to the Compute API Docs, you can't update flavors anyways, so Create and Delete is sufficient, combined with ForceNew: true on any changes :)

jtopjian · 2017-08-25T05:42:07Z

Edit: So according to the Compute API Docs, you can't update flavors anyways, so Create and Delete is sufficient, combined with ForceNew: true on any changes :)

Yes, this is correct. I realized this this evening. While Horizon gives the ability to update flavors, it's really doing a delete/create in the background.

If you guys get around to adding Delete, I'd like to do one final review before it gets merged.

mcanevet · 2017-08-25T06:46:05Z

@fatmcgav @jtopjian , I tried to update gophercloud using govendor update github.com/gophercloud/gophercloud/... but I get compilation errors unrelated with my changes:

 make build
==> Checking that code complies with gofmt requirements...
go install
# github.com/terraform-providers/terraform-provider-openstack/vendor/github.com/gophercloud/gophercloud/openstack/identity/v3/users
vendor/github.com/gophercloud/gophercloud/openstack/identity/v3/users/requests.go:216: cannot use "github.com/terraform-providers/terraform-provider-openstack/vendor/github.com/gophercloud/gophercloud/pagination".LinkedPageBase literal (type "github.com/terraform-providers/terraform-provider-openstack/vendor/github.com/gophercloud/gophercloud/pagination".LinkedPageBase) as type "github.com/gophercloud/gophercloud/pagination".LinkedPageBase in field value
GNUmakefile:7: recipe for target 'build' failed
make: *** [build] Error 2

If I update only the flavor using govendor update github.com/gophercloud/gophercloud/openstack/compute/v2/flavors/... it still compiles (I still have to implement Deletethough.

Any idea? Should I update only openstack/compute/v2/flavors?

jtopjian · 2017-08-25T06:50:34Z

Any idea? Should I update only openstack/compute/v2/flavors?

Yes.

I've been meaning to do a full Gophercloud vendor update and see if I can resolve any errors. But for now, definitely just update the flavors package.

fatmcgav · 2017-08-25T06:50:38Z

@mcanevet Yeh, I think the general practise is to only update the package required...

So just updating flavors is fine.

mcanevet · 2017-08-25T06:52:37Z

I just implemented Delete

fatmcgav

Changes all look good to me.

Will leave for @jtopjian to provide final approval though :)

jtopjian

@mcanevet Thank you for your work on this. Just a few small changes in the code.

Additionally, there needs to be two changes to the acceptance tests:

Since two flavors with the same name can't exist, the acceptance test will fail if there's already a flavor with the name of flavor_1. So we need to randomly generate a flavor name. You can see how to do that in the Projects test:

https://github.com/terraform-providers/terraform-provider-openstack/blob/master/openstack/resource_openstack_identity_project_v3_test.go

A separate test file for the Import functionality needs to be added. A quick copy/paste/rename of the Key Pair test should work:

https://github.com/terraform-providers/terraform-provider-openstack/blob/master/openstack/import_openstack_compute_keypair_v2_test.go

At a minimum, if you can fix the inline comments I made, I'm OK with merging this. Acceptance tests aren't a firm requirement for having code merged, so I can follow-up with the above. But if you want to do it for experience, go for it :)

edit: Actually, the Project Import test is a better example since it requires Admin privileges + does the random name generation:

https://github.com/terraform-providers/terraform-provider-openstack/blob/master/openstack/import_openstack_identity_project_v3_test.go

jtopjian · 2017-08-26T01:44:56Z

openstack/resource_openstack_compute_flavor_v2.go

+
+	disk := d.Get("disk").(int)
+	swap := d.Get("swap").(int)
+	is_public := d.Get("is_public").(bool)


nit: isPublic instead of is_public.

jtopjian · 2017-08-26T01:46:19Z

openstack/resource_openstack_compute_flavor_v2_test.go

+			return err
+		}
+
+		if found.Name != rs.Primary.ID {


found.ID

The test was failing and this took me a 30 minutes to spot 😄

jtopjian · 2017-08-26T01:46:57Z

website/docs/r/compute_flavor_v2.html.markdown

+
+## Import
+
+Flavors can be imported using the `name`, e.g.


s/name/ID

jtopjian · 2017-08-26T01:47:26Z

website/docs/r/compute_flavor_v2.html.markdown

+Flavors can be imported using the `name`, e.g.
+
+```
+$ terraform import openstack_compute_flavor_v2.my-flavor test-flavor


s/test-flavor/4142e64b-1b35-44a0-9b1e-5affc7af1106/

jtopjian · 2017-08-28T17:32:50Z

LGTM

@mcanevet Thank you for submitting this and your patience!

@fatmcgav Thanks for getting the flavor Delete functionality in place!

Add openstack_compute_flavor_v2 resource

mcanevet force-pushed the flavor branch from 3b249ee to 189edfe Compare August 23, 2017 07:23

fatmcgav added the new-resource label Aug 23, 2017

mcanevet force-pushed the flavor branch 2 times, most recently from 8c801c6 to bf37f40 Compare August 24, 2017 09:41

fatmcgav reviewed Aug 24, 2017

View reviewed changes

Update gophercloud/openstack/compute/v2/flavors

e5c7959

mcanevet force-pushed the flavor branch from d73c1c9 to 790b716 Compare August 25, 2017 06:46

mcanevet changed the title ~~[WIP] Add openstack_compute_flavor_v2 resource~~ Add openstack_compute_flavor_v2 resource Aug 25, 2017

mcanevet force-pushed the flavor branch from ed3a293 to 0b721a0 Compare August 25, 2017 06:53

fatmcgav reviewed Aug 25, 2017

View reviewed changes

jtopjian requested changes Aug 26, 2017

View reviewed changes

Add openstack_compute_flavor_v2 resource

1ed0ab7

mcanevet force-pushed the flavor branch from 7c3c453 to 1ed0ab7 Compare August 28, 2017 07:24

jtopjian approved these changes Aug 28, 2017

View reviewed changes

jtopjian merged commit 05f7b78 into terraform-provider-openstack:master Aug 28, 2017

mcanevet deleted the flavor branch August 29, 2017 06:23

gator1 pushed a commit to gator1/terraform-provider-opentelekomcloud that referenced this pull request Nov 27, 2017

Merge pull request terraform-provider-openstack#83 from mcanevet/flavor

548af8a

Add openstack_compute_flavor_v2 resource


		# openstack\_compute\_flavor_v2

		Manages a V2 flavor resource within OpenStack.

Add openstack_compute_flavor_v2 resource #83

Add openstack_compute_flavor_v2 resource #83

Conversation

mcanevet commented Aug 22, 2017 • edited

jtopjian commented Aug 23, 2017 • edited

mcanevet commented Aug 23, 2017

fatmcgav commented Aug 23, 2017

mcanevet commented Aug 23, 2017

fatmcgav commented Aug 23, 2017

mcanevet commented Aug 23, 2017

fatmcgav commented Aug 23, 2017

mcanevet commented Aug 23, 2017

fatmcgav commented Aug 23, 2017

fatmcgav commented Aug 24, 2017

fatmcgav commented Aug 24, 2017

mcanevet commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtopjian commented Aug 24, 2017

fatmcgav commented Aug 24, 2017 • edited

jtopjian commented Aug 25, 2017

mcanevet commented Aug 25, 2017

jtopjian commented Aug 25, 2017

fatmcgav commented Aug 25, 2017

mcanevet commented Aug 25, 2017

fatmcgav left a comment

Choose a reason for hiding this comment

jtopjian left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtopjian commented Aug 28, 2017

mcanevet commented Aug 22, 2017 •

edited

jtopjian commented Aug 23, 2017 •

edited

fatmcgav commented Aug 24, 2017 •

edited

jtopjian left a comment •

edited