-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change rules
from Set to List in detector resource
#378
Change rules
from Set to List in detector resource
#378
Conversation
Due to `rules` being of type Set, we're facing a few issues with using SignalFX TF provider for managing hundreds of our existing detectors. 1. We can't ignore only changes in specific attributes of the `rule`, but rule block as a whole. Taking into account that any threshold change introduced via SignalFX UI or API would diverge the `description` attribute of modified rule to some autogenerated text, it became very hard to keep our Terraform states with 0 drifts. At the end we're used to completely ignore changes in the `rule` block, effectively loosing the possibility to modify other rule attributes from within Terraform code. 2. Another downside of TypeSet is that Terraform plan outputs always show re-creation of all rule objects instead of showing actual diff. For example, if I have 3 rules and wants to change the `disabled` attribute in one of them - TF plan would show that all old rules should be removed and 3 new ones would be added as it can't match old Set items with new ones. P.s. I've built the provider locally & tested it on several existing detectors & internal modules without any compatibility issues noticed. Please let me know if there're some technical constrains to merge this change
@keitwb @mbojko-signalfx Could you please share your feedback on proposed change? Described downsides of |
Proposed change seems to be reasonable, I'll need to run few test to see if it doesn't introduce backward compatibility issues. |
I've had time to verify the proposed change and unfortunately I've noticed backward compatibility issues. |
@@ -288,7 +288,7 @@ func timeRangeStateUpgradeV0(rawState map[string]interface{}, meta interface{}) | |||
*/ | |||
func getPayloadDetector(d *schema.ResourceData) (*detector.CreateUpdateDetectorRequest, error) { | |||
|
|||
tfRules := d.Get("rule").(*schema.Set).List() | |||
tfRules := d.Get("rule").([]interface{}).List() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no List()
method on an interface slice
@@ -790,7 +790,7 @@ func validateProgramTextCondition(d *schema.ResourceDiff, meta interface{}) bool | |||
*/ | |||
func validateProgramText(d *schema.ResourceDiff, meta interface{}) error { | |||
|
|||
tfRules := d.Get("rule").(*schema.Set).List() | |||
tfRules := d.Get("rule").([]interface{}).List() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no List()
method on an interface slice
Closing as this PR has gone stale. Please reopen and update if more work is appropriate. |
We've faced a several issues when started migrating hundreds of existing detectors into Terraform due to
rule
being Set.We can't ignore only changes in specific attributes of the
rule
, but rule block as a whole.Taking into account that any threshold change introduced via SignalFX UI or API would diverge the
description
attribute of modified rule to some autogenerated text, it became very hard to keep our Terraform states with 0 drifts.At the end we're used to completely ignore changes in the
rule
block, effectively loosing the possibility to modify other rule attributes from within Terraform code.Another downside of TypeSet is that Terraform plan outputs always show re-creation of all rule objects instead of showing actual diff.
For example, if I have 3 rules and wants to change the
disabled
attribute in one of them - TF plan would show that all old rules should be removed and 3 new ones would be added as it can't match old Set items with new ones.P.s. I've built the provider locally & tested it on several existing detectors & internally developed modules without any compatibility issues noticed.
Please let me know if there're some technical constrains to merge this change