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

TypeSet Check Heuristic #417

Merged
merged 3 commits into from
Apr 29, 2020
Merged

TypeSet Check Heuristic #417

merged 3 commits into from
Apr 29, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Apr 29, 2020

This change can assist users when enabling the binary driver. Previously any checks that addressed through TypeSet hashes would error confusingly (just claim the resource does not exist) as the binary driver's shimmed state did not represent TypeSet addresses.

A small hack/flag needed to be added onto terraform.State, but the docs clearly state it's not for external use.

style set indexes into a TypeSet when using the
binary driver. Currently the check is skipped, but
it could be changed to error should that be a
better behavior
@appilon appilon requested review from paultyng and a team April 29, 2020 03:14
@kmoe
Copy link
Member

kmoe commented Apr 29, 2020

This is a great UX improvement and fills the gap between binary testing in v1 and the fix for #196 (which I still hope I can land on v1 as well as v2).

If we're concerned about false positives, could we instead run the heuristic and log the warning only if the attribute check fails?

helper/resource/testing.go Outdated Show resolved Hide resolved
Copy link
Contributor

@paultyng paultyng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need this to be like a driver string instead of a flag in v2, once we have debugging, depending on some things, but for v1 at least, this LGTM

@appilon appilon merged commit 3f033d5 into v1-maint Apr 29, 2020
@appilon appilon deleted the set-heuristic branch April 29, 2020 19:02
@ghost
Copy link

ghost commented May 30, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators May 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants