DimensionDataResearch / terraform-provider-ddcloud

Terraform provider for Dimension Data cloud compute.
MIT License
16 stars 13 forks source link

Simplify the UX for CloudControl images in Terraform #66

Closed tintoy closed 7 years ago

tintoy commented 7 years ago

See discussion in #64.

We could change ddcloud_server to have just 3 properties nested in an image property:

So a typical configuration that uses the "Ubuntu 14.04 2 CPU" OS image would be:

resource "ddcloud_server" "my_server" {
  // ...

  image {
    name = "Unbutu 14.04 2 CPU"
  }
}

Or if you wanted to force it to use a customer image named "Ubuntu 14.04 2 CPU":

resource "ddcloud_server" "my_server" {
  // ...

  image {
    name = "Unbutu 14.04 2 CPU"
    type = "customer"
  }
}

@mingsheng36 - what do you think?

mingsheng36 commented 7 years ago

Great! I dont know if this might be too much to ask, but can "image_type" be "computed"?

tintoy commented 7 years ago

I can do it, but only if you don't specify an image type. If you actually specify "auto" then if I reset it during deployment to the actual image type then Terraform will think it's changed and destroy / recreate the server during the next deployment.

I could make the default for image_type an empty string, which implies auto (and remove the value auto as a valid choice there). That way, you either specify it explicitly, or it'll be computed for you. But that means users will need to understand that to auto-detect you don't specify it at all. That's probably OK though, since it's the default behaviour, and you only specify it if you want to override it.

mingsheng36 commented 7 years ago

My little opinion - I see no point or use case where Users need to define whether it is OS image or Client image type (outcome - is to have the server deployed & running). Since you are able to make it "auto" which means it can be computed, should we leave it this way? (not allowing client to define) It looks more elegant, simple and also reduces the chances of human errors. (Putting a name or id with mismatch type)

Whats your view? @tintoy

tintoy commented 7 years ago

Problem is, when you remove the ability to override, someone inevitably comes up with a scenario you can't handle (e.g. creating a customer image with the same name as an OS image). So I'm happy to try to work out what they mean as sensible default behaviour, but experience has taught me that software is hardly ever smart enough to satisfy the user and so they will sometimes want to override the choice my code made for them :)

mingsheng36 commented 7 years ago

Make sense to me. :+1:

mingsheng36 commented 7 years ago

I think the scenario where Users having the same name for Client && Default Images is quite impossible (given the fact that Client Images cannot contain "spaces" and all other Default Images contain spaces in their names). However, there are no official documentation on that and from what I know, spaces might be allowed for Client Images soon (while they are imported from OVF) hence there is still a possibility to what you have mentioned.

Just listing down my thoughts.

tintoy commented 7 years ago

Interesting - hadn’t considered that :)

I think I’m comfortable with the implicit-defaults-that-you-can-explicitly-override approach for now; I think it strikes a decent balance between convenience and correctness.

mingsheng36 commented 7 years ago

Yup. Well-said. esp on "implicit-defaults-that-you-can-explicitly-override". 💯

mingsheng36 commented 7 years ago

Seems like CloudControl API v2.4 just got released to public. Will have to take a look at it and see if it changes the design & implementation.

wninobla commented 7 years ago

Yeah, R&D did that yesterday though the UI stuff is not there yet. That is set for next week in preparation for the formal release.

Regards,

William Ninobla Sr. Solutions Architect Cloud Business Unit Dimension Data


From: Ming Sheng notifications@github.com Sent: Tuesday, November 29, 2016 6:41 AM To: DimensionDataResearch/dd-cloud-compute-terraform Subject: Re: [DimensionDataResearch/dd-cloud-compute-terraform] Consider simplifying the UX for CloudControl images in Terraform (#66)

Seems like CloudControl API v2.4 just got released to public. Would have to take a look at it and see if it changes the design & implementation.

- You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/DimensionDataResearch/dd-cloud-compute-terraform/issues/66#issuecomment-263587773, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AQe9v0jkhqjk-FaxhaA9yaVt2SnjXVxzks5rDDmegaJpZM4K9TT4.

itevomcid

mingsheng36 commented 7 years ago

Thanks for the heads up Will. Really really really... needed the Cluster Awareness feature man.

wninobla commented 7 years ago

Well it's aimed squarely at private deployments for now but it's a good step in the right direction. I talked to product about that all re: public so hope that will come also.

mingsheng36 commented 7 years ago

Question: When I deploy customer images, do I need to specify the type?

The config below gives no error, but if I do not specify the "type" it shows terraform crashed

  image {
    id = "a6b0aaa9-d013-4fbf-8b76-7cfa1cbc035a"
    type = "customer" # crashes when not specified
  }

The config below works fine for default images which I thought was supposed to be the same for customer images

  image {
    name = "Redhat 7 64-bit 2 CPU"
  }
tintoy commented 7 years ago

Definitely shouldn't need to specify it - defaults to auto. Can you post the log?

tintoy commented 7 years ago

Never mind - I can reproduce that. Looks like a pretty stupid bug, will fix shortly.

tintoy commented 7 years ago

BTW, I am considering collapsing the image block to just 2 properties:

resource "ddcloud_server" "server1" {
  # ...
  image = "name|id"
  image_type = "customer" # optional, defaults to "auto"
  # ...
}

If image is a valid GUID, it will treat it as an image Id, otherwise, it will treat it as an image name.

@mingsheng36 @wninobla - what do you reckon? It looks cleaner to me.

PS. Haven't made this change yet, just thinking out loud.

tintoy commented 7 years ago

Ok, @mingsheng36 @wninobla v1.2.0-alpha1 is ready - feel free to play around and let me know what's broken :)

mingsheng36 commented 7 years ago

BTW, I am considering collapsing the image block to just 2 properties

I am fully with you on this. Looks much more cleaner!

tintoy commented 7 years ago

Ok then - that's how it will work in alpha2 :)

mingsheng36 commented 7 years ago

@tintoy Seems like it is still crashing when using Client Images to deploy.

  image = "Ming Redhat 7" # Client Image Name, terraform crashes
  //  image = "a6b0aaa9-d013-4fbf-8b76-7cfa1cbc035a" # Client Image ID, terraform crashes
  //  image = "Redhat 7 64-bit 2 CPU" # Default Image Name, works
  //  image = "dde11c6a-588c-44fb-84d9-64a1d94c8dcc" # Default Image ID, terraform crashes

Terraform - v0.7.13 terraform-provider-ddcloud - v1.2.0-alpha3

tintoy commented 7 years ago

Log?

tintoy commented 7 years ago

Ok, so it looks like it works if you specify image_type="customer" but not if you specify image_type="auto" or omit image_type entirely.

tintoy commented 7 years ago

Ok - will be fixed in the next build.

tintoy commented 7 years ago

v1.2.0-alpha4

tintoy commented 7 years ago

Verified (manually and via automated acceptance tests) in v1.2.0-beta1.