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
fix refresh for resource openstack_dns_recordset_v2 #1581
fix refresh for resource openstack_dns_recordset_v2 #1581
Conversation
The resource was incorrectly failing to read in the actual record values of the recordset resource when Terraform runs a refresh. I believe this change was made due to the designate system imposing its own internal sorting on the record values instead of using the order provided by the user when calling the API. Due to this ordering, and the "records" attribute being a list instead of a set, this would cause the Terraform plan to be unnecessarily unstable until the user reordered their record list to match whatever ordering that designate wants to impose even though the order of the records is actually unimportant. This change converts the "records" attribute to a set since the order of the records doesn't matter and record values should be distinct within a given resource definition anyways. This attribute definition change does not require a new schema version since a list of strings and a set of strings are represented identically in the underlying .tfstate file. Also removed the support for stripping surrounding square brackets off of IPv6 addresses as the functionality could actually prevent other types of record values, such as TXT, being set correctly.
Anyone know what I can do about the 5 cancelled checks? P.S. - @nikParasyr Your attention on this PR would be appreciated since you handled its predecessor. |
@jcarrothers-sap ill try to have a look next week when I get the time. Ci cancelled jobs are running now |
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.
@jcarrothers-sap Thanks for the PR.
LGTM and good work :) sorry for the delay to review this
@@ -3,13 +3,15 @@ | |||
NOTES | |||
|
|||
* Documentation has been updated with sub-categories for easier browsing | |||
* '[]' stripping for IPv6 addresses removed from `openstack_dns_recordset_v2` resource |
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.
Your changes on changelog are not fully following our conventions. That is ok as i will patch them before the release. For future reference, you dont need to update changelog as we do it before a release and updating it triggers a full ci run that is unnecessary.
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.
Thanks for the feedback. I added an entry for this particular change because I thought it important that this particular behaviour difference be explicitly called out in the release notes. I appreciate you taking on making sure they are edited to fit the project conventions.
02db7b6
into
terraform-provider-openstack:main
The resource was incorrectly failing to read in the actual record values of the recordset resource when Terraform runs a refresh. I believe this change was made due to Designate imposing its own internal sorting on the record values instead of using the order provided by the user when calling the API. Due to this ordering, and the "records" attribute being a list instead of a set, this would cause the Terraform plan to be unnecessarily unstable until the user reordered their record list to match whatever ordering that designate wants to impose even though the order of the records is actually unimportant.
This change converts the "records" attribute to a set since the order of the records doesn't matter and record values should be distinct within a given resource definition anyways. This attribute definition change does not require a new schema version since a list of strings and a set of strings are represented identically in the underlying .tfstate file.
Also removed the support for stripping surrounding square brackets off of IPv6 addresses as the functionality could actually prevent other types of record values, such as TXT, being set correctly.