-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Made bigquery table schema diffsuppress func handle nested lists gracefully #4832
Conversation
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190199" |
return false | ||
} | ||
|
||
// Sort the first level of the schema by name so reordering the |
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.
A field can have fields nested inside of it [schema], so we still need to sort within the JSON function still, right? With this change reordering nested fields
fields won't work as I'm reading it.
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, I didn't know about nested fields! I'll make this support both.
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190226" |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190226" |
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190389" |
…t multiple levels
I have triggered VCR tests based on this PR's diffs. See the results here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190411" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccTags You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190412" |
I have triggered VCR tests in RECORDING mode for the following tests that failed during VCR: TestAccBigtableAppProfile_bigtableAppProfileSingleclusterExample|TestAccBigtableAppProfile_bigtableAppProfileMulticlusterExample You can view the result here: "https://ci-oss.hashicorp.engineering/viewQueued.html?itemId=190427" |
Resolved hashicorp/terraform-provider-google#9230. The comparison function was assuming that if the current object was a list, it was the top-level schema list and could be sorted by field name. However, this logic ended up also getting applied to nested lists, which don't have a field name to sort by (thus causing a panic.)
Specifically, bigquery table schema can specify policyTags per-column.
I believe the purpose of sorting by field name is to prevent diffs if fields in the schema are reordered.
We can preserve this logic by pre-sorting the schema by field name prior to passing it to the comparison function.(Note: due to nested fields, ended up going with a different strategy.)If this PR is for Terraform, I acknowledge that I have:
make test
andmake lint
to ensure it passes unit and linter tests.Release Note Template for Downstream PRs (will be copied)