Terraform ProfitBricks Builder #7943
Conversation
|
@jen20 @radeksimko Guys do you have any remarks? |
…r-profitbricks # Conflicts: # command/internal_plugin_list.go
|
@jen20 @radeksimko pinging on this PR. |
|
@mwhooker Is there anything i should do here? |
|
@mwhooker I'm checking in again |
|
ping? |
|
looks like this needs a rebase |
|
Check |
There was a problem hiding this comment.
Hi @jasminSPC
Thanks for all the work here - sorry this has taken so long to get back to! I have just given this a quick once through for some stylistic comments
I have left some inline that could be done with rolling across the entire provider
If you have it, please can you run the acceptance tests and add the output to the PR? :)
Thanks
Paul
| func (c *Config) Client() (string, error) { | ||
| profitbricks.SetAuth(c.Username, c.Password) | ||
| profitbricks.SetDepth("5") | ||
| log.Printf("[DEBUG] Username and password %s : %s", c.Username, c.Password) |
There was a problem hiding this comment.
I don't think we should log the password here
| DefaultFunc: schema.EnvDefaultFunc("PROFITBRICKS_PASSWORD", nil), | ||
| Description: "Profitbricks password for API operations.", | ||
| }, | ||
| "timeout": { |
There was a problem hiding this comment.
Should we set a default timeout?
| config := Config{ | ||
| Username: d.Get("username").(string), | ||
| Password: d.Get("password").(string), | ||
| Timeout: d.Get("timeout").(int), |
There was a problem hiding this comment.
We should check that this is set otherwise, it will be a 0 being passed - is that an issue?
| } | ||
|
|
||
| func resourceProfitBricksDatacenterCreate(d *schema.ResourceData, meta interface{}) error { | ||
| username, password, _ := getCredentials(meta) |
There was a problem hiding this comment.
this feels onerous for someone extending - I think there should be a helper func that takes care of this
conn := getProfitBricksConnection(meta)
| } | ||
|
|
||
| resp := profitbricks.PatchDatacenter(d.Id(), obj) | ||
| waitTillProvisioned(meta, resp.Headers.Get("Location")) |
There was a problem hiding this comment.
wouldn't we have to get Location from state here?
waitTillProvisioned(meta, resp.Headers.Get(d.Get("Location").(string)))
There was a problem hiding this comment.
ProfitBricks REST returns request status url in "Location" response header.
https://devops.profitbricks.com/api/cloud/v3/#how-to-access-the-api
|
|
||
| return fmt.Errorf("Request failed with following error: %s", request.Metadata.Message) | ||
| } | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
why do we have to sleep here?
There was a problem hiding this comment.
While polling the request status every iteration we sleep for 10 seconds.
| if err != nil { | ||
| return err | ||
| } | ||
| return nil //resourceProfitBricksFirewallRead(d, meta) |
There was a problem hiding this comment.
Update should call Read func as a general rule to make sure all the state values get set as expected
| resp := profitbricks.DeleteLan(d.Get("datacenter_id").(string), d.Id()) | ||
| if resp.StatusCode > 299 { | ||
| //try again in 20 seconds | ||
| time.Sleep(60 * time.Second) |
There was a problem hiding this comment.
Comment above this says 20 seconds - can we clean that up - do we have to sleep?
| d.Set("ip", lb.Properties.Ip) | ||
| d.Set("dhcp", lb.Properties.Dhcp) | ||
| if lb.Entities.Balancednics != nil && len(lb.Entities.Balancednics.Items) > 0 { | ||
| d.Set("nic_id", lb.Entities.Balancednics.Items[0].Id) |
There was a problem hiding this comment.
Can we guarantee that the NIC at [0] is the correct one?
There was a problem hiding this comment.
Since "nic_id" is a required field it is not necessary to set it here. removed
|
I've addressed all remarks. |
|
@jen20 @radeksimko @stack72 Is there anything else required to have this PR merged? |
|
Hi @jasminSPC I will get this reviewed and merged next week! Please ping me if you haven't heard from me before Tuesday end of day :) P. |
|
@stack72 Ping. It's end of the day where I live :) |
|
@jasminSPC on this now :) |
|
Hi @jasminSPC This LGTM now - I will need to get a ProfitBricks account setup so that I can test this out. I will have that done today - when the folks on the US West Coast wake up :) Thanks for rounding off those changes Paul |
|
@stack72 We've added three data sources to our third party plugin. Do you think it would be good to add it now. As for the credentials we can provide you those. Can you just email me so I can send it over to you. |
|
ok, let's follow up with the datasources as another PR - I don't want to delay this anymore :) If you have creds for testing, can you email them to stack72 @ hashicorp.com P. |
|
Hi @jasminSPC Ok, I have ran the tests and we are getting failures. The test run looks as follows: Can you have a look? Paul |
stack72
added
the
waiting-response
|
@stack72 we were actually planning to remove imports. Do you mind if I remove them now? |
|
ok, let's remove import for now :) We can revisit that later |
|
Give me a minute to wrap it up and sync the fork |
|
👍 I am on this PR today - I promised :) |
|
@stack72 There you go |
|
thanks @jasminSPC Testing again now! |
|
Hi @jasminSPC This LGTM! now :) |
|
Thansk for all the work here :) |
* master: (232 commits) Update CHANGELOG.md command/plugin_list: Adding Alicloud to the plugin list file (#11292) Additionnal information for machine type (#11288) provider/alicloud: Creating the necessary structure for the alicloud documentation (#11289) Update CHANGELOG.md update alicloud provider (#11235) Update CHANGELOG.md provisioner/chef: Support named run-lists for Policyfiles (#11215) Correct data.aws_route_table filter AWS docs link Update CHANGELOG.md Update CHANGELOG.md removes region param from google_compute_backend_service (#10903) core: Add pathexpand interpolation function Update CHANGELOG.md Add instance_tags as an additional filter provider/aws: Add aws_instance data source Update `parameter_group_name` (#11269) provider/profitbricks: Rename the profitbricks bin so that the plugin (#11267) Update CHANGELOG.md Terraform ProfitBricks Builder (#7943) ...
|
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. |
ghost
locked and limited conversation to collaborators
This is terraform builder for ProfitBricks.
While writing unit tests I've ran on to a issue with a computed field
primary_nicinprofitbricks_serveras suggested by the output this is a Terraform bug. I'm pasting the output here:The property
primary_nicis computed when a server is created this doesn't cause any problems when using the builder as a third-party builder. Also ifidofprofitbricks_nicis used this issue doesn't exist. It seems thatapplydoesn't pick up this property.I also tried writing unit tests for import but I kept running into problems. For example if I'm to import a
profitbricks_lanthedatacenter_idon whichprofitbricks_landepends, butdatacenter_idvalue wouldn't be passed on to read function. Also some properties of other resources don't get recorded intotfstate.Can you advise us on how to fix that?
profitbricks_loadbalancerresource doesn't function correctly due to a known issue with the REST API. It is due to be released in upcoming releases.