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

command/format: Fix rendering of attribute-agnostic diagnostics #19453

Merged
merged 3 commits into from
Nov 26, 2018

Conversation

radeksimko
Copy link
Member

Depends on hashicorp/hcl2#58

Before

screen shot 2018-11-23 at 17 14 47

After

screen shot 2018-11-23 at 17 22 11


I'm thinking maybe we should suppress the "context string" to avoid the duplication, i.e.

  on /var/workspace/tf-test/repro-case/main.tf line 21:
  21: resource "aws_instance" "web" {

but then we might need the context in the future, more complex cases when we figure out how to render snippets with interpolated count.index? I'm not sure.


It is worth mentioning that there is currently no context rendered for errors returned during refresh or import, because the relevant EvalNode implementations (e.g. EvalRefresh) don't have access to config at all, so there is no way to match it at this point.

If we want to avoid passing config to these EvalNodes we might need to consider adding context in the form of IDs or something like that. 🤔

With all that said this is a problem to be solved in a future PR.

@radeksimko radeksimko added the bug label Nov 23, 2018
@radeksimko radeksimko requested a review from a team November 23, 2018 17:30
Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

w.r.t. the errors during state-only operations like refresh, indeed even if we did pass configuration in there we can't be sure it'd always be present since we perform refresh/destroy calls on objects that are no longer in the configuration. I do think there'll need to be some further iteration on this later, but what you've done here at least demonstrates that the part of the problem that lives within the SDK (and thus the provider binaries) is now working as expected and so we can work on further annotating the diagnostics in later Terraform Core releases without any protocol changes or forced provider upgrades.

@ghost
Copy link

ghost commented Mar 31, 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 Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants