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

Eval diagnostics #26738

Merged
merged 12 commits into from
Oct 28, 2020
Merged

Eval diagnostics #26738

merged 12 commits into from
Oct 28, 2020

Conversation

jbardin
Copy link
Member

@jbardin jbardin commented Oct 28, 2020

This completes the diagnostics portion of the Eval node refactor. Diagnostics are now maintained from the RPC calls all the way through core error handling. The most noticeable external benefit to this is that warnings from plugins can now be passed all the way through to the UI even when there are no accompanying errors for them to piggy-back on.

We do have a change in behavior to note however. Not using raw errors during eval makes it much harder to hide the error that was being used to stop execution when there's an interrupt. This however may not be an issue, since a partial execution should not result in a 0 exit code, and most unix process in general exit with a non-0 code when interrupted. Merging this early in the 0.15 development cycle gives us time to evaluate the change, and assess if we need to work on another method to hide that error.

Also closes #26680

This was mostly unused now, since we no longer needed to interrupt a
series of eval node executions.

The exception was the stopHook, which is still used to halt execution
when there's an interrupt. Since interrupting execution should not
complete successfully, we use a normal opaque error to halt everything,
and return it to the UI.

We can work on coalescing or hiding these if necessary in a separate PR.
@jbardin jbardin requested a review from a team October 28, 2020 18:57
@codecov
Copy link

codecov bot commented Oct 28, 2020

Codecov Report

Merging #26738 into master will increase coverage by 0.00%.
The diff coverage is 50.66%.

Impacted Files Coverage Δ
terraform/eval_context_builtin.go 73.88% <0.00%> (-0.75%) ⬇️
terraform/eval_validate.go 64.58% <ø> (+7.96%) ⬆️
terraform/node_data_destroy.go 100.00% <ø> (ø)
terraform/node_root_variable.go 66.66% <ø> (ø)
terraform/eval_validate_selfref.go 79.31% <20.00%> (-5.88%) ⬇️
terraform/node_output.go 76.28% <27.27%> (-2.97%) ⬇️
terraform/eval_read_data_apply.go 57.14% <31.25%> (+0.73%) ⬆️
terraform/node_count_boundary.go 73.07% <33.33%> (-2.93%) ⬇️
terraform/eval_state.go 51.39% <37.20%> (-2.22%) ⬇️
terraform/node_resource_apply_instance.go 75.00% <37.50%> (-0.80%) ⬇️
... and 30 more

Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Geez @jbardin who opens PRs like this, it's way too big (❤️)

I really like this! The terraform package gets more readable with every pass, AND you took care of something I'd been hoping to get to 🎉

@apparentlymart
Copy link
Member

FWIW, I'm generally positive towards "being cancelled is an error", and consider that to be better behavior even if it's not what it used to be doing. 🤷‍♂️

@jbardin jbardin merged commit d1ac382 into master Oct 28, 2020
@jbardin jbardin deleted the jbardin/eval-diagnostics branch October 28, 2020 21:21
@ghost
Copy link

ghost commented Nov 28, 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 as resolved and limited conversation to collaborators Nov 28, 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.

Diagnostic warnings not shown in Configure, Read when no errors
3 participants