Skip to content
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

Merged
merged 2 commits into from Jun 19, 2023

Conversation

jcarrothers-sap
Copy link
Contributor

@jcarrothers-sap jcarrothers-sap commented Jun 12, 2023

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.

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.
@jcarrothers-sap
Copy link
Contributor Author

jcarrothers-sap commented Jun 12, 2023

This is a follow up PR to #1566 and a more complete and correct fix for #1564

@jcarrothers-sap
Copy link
Contributor Author

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.

@nikParasyr
Copy link
Member

@jcarrothers-sap ill try to have a look next week when I get the time. Ci cancelled jobs are running now

Copy link
Member

@nikParasyr nikParasyr left a 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
Copy link
Member

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.

Copy link
Contributor Author

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.

@nikParasyr nikParasyr merged commit 02db7b6 into terraform-provider-openstack:main Jun 19, 2023
62 checks passed
@jcarrothers-sap jcarrothers-sap deleted the patch-1 branch June 19, 2023 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants