search

RavinderReddyF5 / terraform-provider-bigip-version0.12

Terraform resources that can configure F5 BIGIP products
Mozilla Public License 2.0
0 stars 0 forks source link

[CLOSED] Default description should not include "Test" string #179

Open RavinderReddyF5 opened 4 years ago

RavinderReddyF5 commented 4 years ago

Issue by fcgravalos Friday Aug 16, 2019 at 08:58 GMT Originally opened as https://github.com/terraform-providers/terraform-provider-bigip/issues/148

I think that default description for any object should have the "Test" string as a default value. The default should be an empty string.

We applied a change and didn't notice the update in the plan and now we have descriptions for pools and nodes that contains "Test", like "VirtualServer-Test", this is really confusing it should just not have description at all.

cc @dannyk81 @scshitole

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Friday Aug 16, 2019 at 10:57 GMT

agree, this is wrong...

https://github.com/terraform-providers/terraform-provider-bigip/blob/3976a65261a984b23961963cee256eec08ffeee9/bigip/resource_bigip_ltm_node.go#L63-L67

This was introduced in the following PR: https://github.com/terraform-providers/terraform-provider-bigip/pull/128

/cc @RavinderReddyF5 - this needs to be fixed.

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Friday Aug 16, 2019 at 11:00 GMT

this attribute (since it's Optional), should not have the Default parameter at all.

RavinderReddyF5 commented 4 years ago

Comment by jlosito Friday Aug 16, 2019 at 11:53 GMT

I'm currently having the same issue with my F5 instance. Now I have a bunch of entries with the *-test description field. Below describes how I'm trying to work around it.

I'm observing a weird issue with this description attribute where only upon the second terraform apply does it update the description to nil. I'm currently trying to override this default description field by adding a variable whose default value is "". Something like the following

variable "vip_description" {
  description = "The description field for the VIP."
  type = "string"
  default = ""
}

resource "bigip_ltm_virtual_server" "my_vip" {
  name = "/Partition/my_vip"
  ...
  description = "${var.vip_description}"
}

What I'm observing is upon the first terraform apply, it will still set the VIP's description field to "VirtualServer-test" despite the variable named vip_description and it evaluating to the empty string. Upon the second terraform apply, without changing any source code or values of variables, it then updates the description to nil. The same applies for other resources such as nodes and pools.

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Friday Aug 16, 2019 at 15:13 GMT

@jlosito that seems to be related with the fact that the VS, Pool and Node resoources Read method doesn't update the description field... this seems to be causing these weird artifacts.

The Update method calls the Read method at the end of the run, which is how the State gets updated, but the Read method doesn't have a d.Set() call for the description attribute which is critical for this to work properly.

This may not be the root cause, but I think it is and should be fixed anyway as it makes the implementation of this attributes incomplete.

@RavinderReddyF5 @scshitole FYI

There was a similar issue reported in the past about similar behaviour https://github.com/terraform-providers/terraform-provider-bigip/issues/43, the use of Default is a bit tricky and counter intuitive and in fact should be used with care, for sure it has no place here for the description field.

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Monday Aug 19, 2019 at 09:10 GMT

Hi, @dannyk81 @jlosito @scshitole

Please review Changes in Pull request: https://github.com/terraform-providers/terraform-provider-bigip/pull/151

RavinderReddyF5 commented 4 years ago

Comment by scshitole Tuesday Aug 20, 2019 at 15:40 GMT

@fcgravalos can we close this now

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Tuesday Aug 20, 2019 at 16:10 GMT

@scshitole was this tested? does it fully fix the issue?

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Tuesday Aug 20, 2019 at 16:23 GMT

@dannyk81 , Yea i have tested all scenarios

=== RUN TestAccBigipLtmNode_create --- PASS: TestAccBigipLtmNode_create (1.10s) === RUN TestAccBigipLtmNode_import --- PASS: TestAccBigipLtmNode_import (1.54s) === RUN TestAccBigipLtmNodeInvalid --- PASS: TestAccBigipLtmNodeInvalid (0.02s) === RUN TestAccBigipLtmNodeCreate --- PASS: TestAccBigipLtmNodeCreate (0.10s) === RUN TestAccBigipLtmPool_create --- PASS: TestAccBigipLtmPool_create (0.74s) === RUN TestAccBigipLtmPool_import --- PASS: TestAccBigipLtmPool_import (0.76s) === RUN TestAccBigipLtmVS_create --- PASS: TestAccBigipLtmVS_create (8.47s) === RUN TestAccBigipLtmVS_import --- PASS: TestAccBigipLtmVS_import (4.16s)

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Tuesday Aug 20, 2019 at 16:38 GMT

well, I was going to comment on #151 that the current tests don't actually cover all the scenarios, they only test empty descriptions...

but you went ahead and merged it...

RavinderReddyF5 commented 4 years ago

Comment by scshitole Tuesday Aug 20, 2019 at 16:51 GMT

@dannyk81 can you please specify the test cases you would like to cover, we can test those before releasing the binary ?

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Tuesday Aug 20, 2019 at 17:16 GMT

RavinderReddyF5 commented 4 years ago

Comment by dannyk81 Thursday Aug 29, 2019 at 12:49 GMT

@scshitole @RavinderReddyF5 are you planning to add these tests? if not, I suppose this issue can be closed as fixed...

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Thursday Aug 29, 2019 at 14:01 GMT

@dannyk81 , Will be going to add, will create separate ticket for same.

RavinderReddyF5 commented 4 years ago

Comment by RavinderReddyF5 Thursday Nov 07, 2019 at 08:43 GMT

Fixed in Latest Release