provider/clc: CenturyLink Cloud Provider #4893

ack · 2016-01-28T23:13:36Z

initial PR for provider integration, ready for feedback.

driver
docs
example config
pending acc test account

phinze · 2016-02-27T18:58:25Z

builtin/providers/clc/resource_clc_group.go

+		log.Printf("[INFO] Using EXISTING group: %v => %v", name, m[name])
+		d.SetId(m[name])
+		return nil
+	}


A bit more Go-style way to do this would be:

if id, ok := m[name]; ok { log.Printf("...") d.SetId(id) return nil }

Ah and actually we shouldn't have this "use existing group" behavior in Create - it's inconsistent with the way resource creates generally work in Terraform. We'll implement import functionality as a first class feature. Attempting to create a resource that already exists should result in an error.

Part of the intent behind this is the relative inconvenience of working with server groups on clc. In particular, I was trying to allow the user to refer to a group by name rather than the clunkier guid. If instead the code errors on existing group in create, that would more or less require the user to either a) manage groups explicitly or b) book-keep guids. Not sure if the intention comes across through this description. But in the end, if it runs against convention, I'll remove it.

Thanks for the extra context here.

If you want to enable users who are not managing server groups in Terraform to be able to reference them from Terraform, it seems to me that the name vs guid question is more of an issue in the referencing resources rather than this group resource itself.

The behavior you get by rescuing this error is a "poor man's terraform import". If you assume at some future date that a user will be able to do something like terraform import clc_group.mygroup "groupfoo" - this "pull group under management on collision error during create" behavior ends up creating confusion.

Does that make sense?

yes. i'll have it error and await the import directive. thanks for clarifying.

hmm, so I revisited this problem and it's a bit thorny. here are the details:

there's no way to create a new toplevel group and every group needs a parent

parent_group_id is required on create

groups don't have any uniqueness by attribute. eg. you can have multiple groups named foo (under the same parent). so it's not possible to create a resource (with some name) that already exists, a duplicate will always be created.

the tests may need a hard-coded guid

import is not yet available

putting this altogether, it seems to me that having a poor man's import would be ideal. it's entirely possible I'm not understanding the intended pattern in the conventional way. would it be so horrible to break the rules here?

Ah yes - thorny indeed!

I do see the merits in argument you're making, but the fact that groups don't have any uniqueness constraint makes me worry. What would the behavior be when there are two groups named "foo"?

Anyways, all this said - I'm willing to let this behavior stand for the initial implementation. We can let the implementation evolve with more usage.

Ya, there would be 2 groups with the same name and different IDs. Kinda
silly but that's how it works. I'll tidy up the rest of the group bits and
we can revisit later.
On Mar 2, 2016 3:58 PM, "Paul Hinze" [email protected] wrote:

In builtin/providers/clc/resource_clc_group.go
#4893 (comment):

+}
+
+func resourceCLCGroupCreate(d *schema.ResourceData, meta interface{}) error {

client := meta.(*clc.Client)

dc := d.Get("location_id").(string)

m, err := dcGroups(dc, meta)

if err != nil {

return fmt.Errorf("Failed pulling groups in location %v - %v", dc, err)

}

name := d.Get("name").(string)

// use an existing group if we have one

if m[name] != "" {

log.Printf("[INFO] Using EXISTING group: %v => %v", name, m[name])

d.SetId(m[name])

return nil

}

Ah yes - thorny indeed!

I do see the merits in argument you're making, but the fact that groups
don't have any uniqueness constraint makes me worry. What would the
behavior be when there are two groups named "foo"?

Anyways, all this said - I'm willing to let this behavior stand for the
initial implementation. We can let the implementation evolve with more
usage.

—
Reply to this email directly or view it on GitHub
https://github.com/hashicorp/terraform/pull/4893/files#r54814841.

@ack

it might not be optimal, but I worked around this by using the top-level DC group…

So parent = "GB3 Hardware”.

Could that be a sensible fall back?

Cheers
Gav

On 2 Mar 2016, at 23:35, albert [email protected] wrote:

In builtin/providers/clc/resource_clc_group.go #4893 (comment):

+}
+
+func resourceCLCGroupCreate(d *schema.ResourceData, meta interface{}) error {

client := meta.(*clc.Client)

dc := d.Get("location_id").(string)

m, err := dcGroups(dc, meta)

if err != nil {

return fmt.Errorf("Failed pulling groups in location %v - %v", dc, err)

}

name := d.Get("name").(string)

// use an existing group if we have one

if m[name] != "" {

log.Printf("[INFO] Using EXISTING group: %v => %v", name, m[name])

d.SetId(m[name])

return nil

}
hmm, so I revisited this problem and it's a bit thorny. here are the details:

there's no way to create a new toplevel group and every group needs a parent
parent_group_id is required on create
groups don't have any uniqueness by attribute. eg. you can have multiple groups named foo (under the same parent). so it's not possible to create a resource (with some name) that already exists, a duplicate will always be created.
the tests may need a hard-coded guid
import is not yet available
putting this altogether, it seems to me that having a poor man's import would be ideal. it's entirely possible I'm not understanding the intended pattern in the conventional way. would it be so horrible to break the rules here?

—
Reply to this email directly or view it on GitHub https://github.com/hashicorp/terraform/pull/4893/files#r54812368.

phinze · 2016-02-27T19:16:17Z

Hello hello! I began a detailed code review - have to step AFK for now, but I shall return to complete it!

Feel free to follow up on any questions or concerns. 👍

ack · 2016-02-27T21:52:22Z

Thanks for following through and the detailed feedback! Will hop on these
revisions and comment inline.
On Feb 27, 2016 11:16 AM, "Paul Hinze" [email protected] wrote:

Hello hello! I began a detailed code review - have to step AFK for now,
but I shall return to complete it!

Feel free to follow up on any questions or concerns. [image: 👍]

—
Reply to this email directly or view it on GitHub
#4893 (comment).

ack · 2016-03-01T18:21:52Z

Again, thanks for the extra set of eyes, style pointers and feedback. There are some pending bits I need to provide and I'd like your thoughts on a few minor points. Re: err-in-create-on-existing as some help with one of the tests.

ack · 2016-03-01T18:24:07Z

This test has a step that is repeatedly failing. It is testing an update operation and after the update, re-reading my resource fails with a "Not found" (in the resource tree)

https://github.com/ack/terraform/blob/9c17a2dbc8255924878e25d1ba24a6319b8b185e/builtin/providers/clc/resource_clc_load_balancer_test.go#L38

The test errors (I think) here:
https://github.com/ack/terraform/blob/9c17a2dbc8255924878e25d1ba24a6319b8b185e/helper/resource/testing.go#L360

Any clue what I'm doing wrong here or pointers to debugging it?

fatmcgav · 2016-03-02T09:51:47Z

@ack Would be great to get the CLC provider into Terraform core...

Could you take a look at CenturyLinkCloud/terraform-provider-clc#4 and CenturyLinkCloud/terraform-provider-clc#5 with the associated PR's before this gets merged though?

fatmcgav · 2016-03-02T09:56:06Z

examples/clc/variables.tf

+  default = "achoi"
+}
+variable "clc_password" {
+  default = "T*5upfac"


@ack Are these "valid" credentials?

phinze · 2016-03-02T17:23:20Z

@ack Trying to help out with that test question but I can't seem to get the base test to pass.

I think I've got the authentication sorted out, but I can't seem to figure out how to get around this 500 that the API is throwing:

=== RUN   TestAccLoadBalancerBasic
--- FAIL: TestAccLoadBalancerBasic (1.84s)
        testing.go:148: Step 0 error: Error applying: 1 error(s) occurred:

                * clc_load_balancer.acc_test_lb: Failed creating load balancer under WA1/acc_test_lb: http err: [500 Internal Server Error] - {"message":"An error has occurred."}
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/clc    1.852s

Any clues? My test account is ZZ9F

ack · 2016-03-02T18:36:24Z

@phinze there were some set up steps to take on the account (create a network and a server) before the LB could be created. The complete message visible in our control portal was "Group could not be created: No private IP addresses." Also created an account for myself under ZZ9F to duplicate failed tests in the future. Apologies for the hiccup, please pull latest and give it a go now.

phinze · 2016-03-02T18:41:30Z

Thanks @ack! Will give it another go.

there were some set up steps to take on the account (create a network and a server) before the LB could be created.

Can we document whatever the required steps are to get from a fresh account to being able to run acceptance tests on the provider page?

phinze · 2016-03-02T19:26:53Z

Looks like the load balancer "not found" message was just a typo: s/clc_load_balancher.acc_test_lb/clc_load_balancer.acc_test_lb/

Easy fix! 😀

ack · 2016-03-03T21:57:45Z

added an update and some cleanup on groups. thx for the typo catch btw. ready for another pass. have you been able to run the full suite yet?

phinze · 2016-03-20T19:33:49Z

Been working on getting this merge-able, and I believe I'm nearly there.

Here's my branch: https://github.com/hashicorp/terraform/tree/phinze/clc

In the latest commit on that branch I:

Update resource.Retry invocations for new API
Vendor the CLC SDK
Update the provider client initialization to match the API of the latest SDK

But now I can't seem to get the load balancer tests to pass:

=== RUN   TestProvider
--- PASS: TestProvider (0.00s)
=== RUN   TestProvider_impl
--- PASS: TestProvider_impl (0.00s)
=== RUN   TestAccGroupBasic
--- PASS: TestAccGroupBasic (80.35s)
=== RUN   TestAccLBPoolBasic
--- FAIL: TestAccLBPoolBasic (345.72s)
        testing.go:148: Step 0 error: Error applying: 1 error(s) occurred:

                * clc_load_balancer.acc_test_lbp: Failed creating load balancer under WA1/acc_test_lb: http err: [500 Internal Server Error] - {"message":"An error has occurred."}
=== RUN   TestAccLoadBalancerBasic
--- FAIL: TestAccLoadBalancerBasic (1.69s)
        testing.go:148: Step 0 error: Error applying: 1 error(s) occurred:

                * clc_load_balancer.acc_test_lb: Failed creating load balancer under WA1/acc_test_lb: http err: [500 Internal Server Error] - {"message":"An error has occurred."}
=== RUN   TestAccPublicIPBasic
kjkj


--- PASS: TestAccPublicIPBasic (824.65s)
=== RUN   TestAccServerBasic
--- PASS: TestAccServerBasic (480.40s)
FAIL
exit status 1
FAIL    github.com/hashicorp/terraform/builtin/providers/clc    1732.839s

@ack Any idea what might be going on here?

+examples +docs for clc

ack · 2016-03-21T22:52:17Z

Ya, it's a known bug. The workaround was the mysterious ADMIN server that was in your account that I think you tore down. Basically, a server is required in the DC before a LB can be provisioned. I neglected to mention that the last time. If an operation failed w/ no helpful error message, you're likely to get a better one in the UI if you try the same operation.There's a system VPN server in IL1 that we can use permanently.

I've pulled your changes, updated the tests to point to IL1, merged the latest sdk changes, and rebased the branch down to a few commits.

phinze · 2016-03-21T23:28:03Z

Cool thanks for the extra context there - that makes sense. This looks good to go! 🚀

Basically, a server is required in the DC before a LB can be provisioned.

Can we follow up with a modification to the LB tests so they just create their own servers?

phinze · 2016-03-21T23:28:40Z

( Going to land this as-is though so it makes it in .14 👍 - great work on this! 😀 )

provider/clc: CenturyLink Cloud Provider

ack · 2016-03-21T23:46:50Z

awesome! thanks so much for all your support getting this in.

ghost · 2020-04-27T02:05:58Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

ack force-pushed the clc branch from 44f7ef4 to 342a19f Compare January 28, 2016 23:14

radeksimko added enhancement wip labels Jan 29, 2016

phinze removed the wip label Feb 9, 2016

radeksimko added the new-provider label Feb 25, 2016

phinze reviewed Feb 27, 2016
View reviewed changes

fatmcgav reviewed Mar 2, 2016
View reviewed changes

ack force-pushed the clc branch from 6d43fd6 to f0bb7de Compare March 2, 2016 17:10

ack force-pushed the clc branch 2 times, most recently from ecdf6d2 to f75710e Compare March 3, 2016 21:57

This was referenced Mar 4, 2016

inventory adapter for clc mantl/terraform.py#36

Merged

terraform assets and support for CenturyLinkCloud mantl/mantl#1095

Merged

ack and others added 2 commits March 21, 2016 08:58

snapshot from CenturyLinkLabs/terraform-provider-clc

7775cc8

+examples +docs for clc

provider/clc: vendor deps, update to match latest SDK API

c4b2322

ack force-pushed the clc branch from 4f4acbe to 3530e6f Compare March 21, 2016 21:45

ack added 2 commits March 21, 2016 15:34

bump vendored sdk, remove extra fields

4c4be81

set dc in tests

3971f76

ack force-pushed the clc branch from 3530e6f to 3971f76 Compare March 21, 2016 22:51

phinze changed the title ~~[WIP] clc provider~~ clc provider Mar 21, 2016

phinze changed the title ~~clc provider~~ provider/clc: CenturyLink Cloud Provider Mar 21, 2016

phinze added a commit that referenced this pull request Mar 21, 2016

Merge pull request #4893 from ack/clc

b2d3f92

provider/clc: CenturyLink Cloud Provider

phinze merged commit b2d3f92 into hashicorp:master Mar 21, 2016

ghost locked and limited conversation to collaborators Apr 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

provider/clc: CenturyLink Cloud Provider #4893

provider/clc: CenturyLink Cloud Provider #4893

ack commented Jan 28, 2016

phinze Feb 27, 2016

phinze Feb 27, 2016

ack Mar 1, 2016

phinze Mar 2, 2016

ack Mar 2, 2016

ack Mar 2, 2016

phinze Mar 2, 2016

ack Mar 3, 2016

fatmcgav Mar 3, 2016

phinze commented Feb 27, 2016

ack commented Feb 27, 2016

ack commented Mar 1, 2016

ack commented Mar 1, 2016

fatmcgav commented Mar 2, 2016

fatmcgav Mar 2, 2016

phinze commented Mar 2, 2016

ack commented Mar 2, 2016

phinze commented Mar 2, 2016

phinze commented Mar 2, 2016

ack commented Mar 3, 2016

phinze commented Mar 20, 2016

ack commented Mar 21, 2016

phinze commented Mar 21, 2016

phinze commented Mar 21, 2016

ack commented Mar 21, 2016

ghost commented Apr 27, 2020

provider/clc: CenturyLink Cloud Provider #4893

provider/clc: CenturyLink Cloud Provider #4893

Conversation

ack commented Jan 28, 2016

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

phinze commented Feb 27, 2016

ack commented Feb 27, 2016

ack commented Mar 1, 2016

ack commented Mar 1, 2016

fatmcgav commented Mar 2, 2016

Choose a reason for hiding this comment

phinze commented Mar 2, 2016

ack commented Mar 2, 2016

phinze commented Mar 2, 2016

phinze commented Mar 2, 2016

ack commented Mar 3, 2016

phinze commented Mar 20, 2016

ack commented Mar 21, 2016

phinze commented Mar 21, 2016

phinze commented Mar 21, 2016

ack commented Mar 21, 2016

ghost commented Apr 27, 2020