-
Notifications
You must be signed in to change notification settings - Fork 233
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
Remove TF_SCHEMA_PANIC_ON_ERR #462
Conversation
@@ -18,11 +18,6 @@ import ( | |||
) | |||
|
|||
func init() { | |||
// TODO: Remove when we remove the guard on id checks |
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.
This is not referenced anywhere else
oldEnv := os.Getenv(PanicOnErr) | ||
|
||
os.Setenv(PanicOnErr, "false") | ||
os.Setenv("TF_ACC", "") |
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.
oh god this is just asking for a race condition problem 😭
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.
Agreed, these unit tests aren't super high value so we could ditch the tests if you find this too "anti pattern"
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.
LGTM, should have someone else weigh in as well
Schema errors will panic if TF_ACC is set, it is no longer on by default but cannot be opted out of when running acceptance tests.
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.
It would make me very happy if we at least logged the error, from the SDK level, to ensure that if we're going to fail quietly, we at least make some sound about it, even if the user doesn't see it, so it's not discovered an hour into a debug session. We know what happened, we know exactly what went wrong, we should just put it in the logs as an error or warning level, to leave a trail for whatever poor soul has to puzzle through a bug report.
@paddycarver no problem will do. |
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. |
Schema errors will panic if TF_ACC is set, panicking is no longer on by default
but cannot be opted out of when running acceptance tests. This does not address #455 .
Technically a breaking change
schema.PanicOnErr
is no longer an exported varCloses #451