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

Add resource address to error message #60

Closed
mrzor opened this issue Jul 24, 2020 · 3 comments · May be fixed by #62
Closed

Add resource address to error message #60

mrzor opened this issue Jul 24, 2020 · 3 comments · May be fixed by #62

Comments

@mrzor
Copy link

mrzor commented Jul 24, 2020

When having multiple shell_script resources sharing the same name (i.e. in a module instanciated several times), it's hard to know which one is yielding an error because Terraform displays the error at the end without context.

Current

Error: Error occurred during execution.
 Command: './redacted read' 
 Error: 'exit status 127' 
 StdOut: 
  
 StdErr: 
 /bin/sh: 1: ./redacted: not found

One can guess it is the last shell_script resources to be read in that case.

I imagine that with parallelism turned on, such guessing becomes even harder.

Suggested

Example:

Error: Error occurred during execution.
Resource: module.mymodule.shell_script.mything
 Command: './redacted read' 
 Error: 'exit status 127' 
 StdOut: 
  
 StdErr: 
 /bin/sh: 1: ./redacted: not found
@mrzor mrzor changed the title Add resource adress to error message Add resource address to error message Jul 24, 2020
@scottwinkler
Copy link
Owner

I think this is a great idea, and I spent some time looking into this but I really don't think its possible. Terraform providers operate in a sandbox and don't have very much information available to them except the minimum they need to be able to do their job. As such, you know the InstanceState of resources, which may include information such as you would find in the "instance" attribute of the state file, e.g.

{
          "schema_version": 0,
          "attributes": {
            "environment": null,
            "id": "bsehc7dgrkrh05uual20",
            "interpreter": null,
            "lifecycle_commands": [
              {
                "read": "echo \"{\\\"user\\\": \\\"$(whoami)\\\"}\"\n"
              }
            ],
            "output": {
              "user": "swinkler"
            },
            "sensitive_environment": null,
            "working_directory": "."
          }
        }

As you can see, the id in the instances is not the same as the resource address. Worse, it is only set after Create() has successfully completed, meaning that you could only print it in the error message for Read(), Update() and Delete(). I guess this could still be beneficial, but it could also be confusing and frustrating not to have the error message for Create(), which IMO is the most important.

There does not appear to be any way to the resource address, as far as I can tell. Resources only have *schema.ResourceData available to them, which does contain a reference to State(), but again this is InstanceState and not the complete state file. ResourceAddress could be gleaned from InstanceInfo, but no reference to this struct is present via *schema.ResourceData. Even if you were to do something janky like read the configuration code or state file through alternative means, you still wouldn't know exactly what the resource address is for the resource in question.

Might I suggest using a for_each on the shell resource/module instead? this might make it more clear where the error message is coming from. Resources in for_each are addressed as: module.shell_script.example["a"] where "a" is an element in the set used to instantiate the resources.

@mrzor
Copy link
Author

mrzor commented Jul 26, 2020

I now understand the limitations the provider has to work under. I think an argument could be made to terraform devs to expand the context a bit for error messages - or they could provide the context when displaying the error - because Terraform displays the error, the provider merely formats it. That would be a proper fix.

Using for_each looks like a good workaround.

I thing the workaround has some downsides (I'll admit they're pretty weak):

  • It's subversive! for_each is meant to iterate on several things, as the documentation clearly states.
  • It makes the address longer and redundant - it would be module.mymodinstance.shell_script.example["mymodinstance"]
  • References to my shell script resource now either have to use the longer address, or a local - which makes the config harder to grasp.
  • My experience with the count = 0 kludge - which is similar in principle - is that one should be wary of unchecked dynamic addressing, as it can fail in perplexing ways in some contexts like terraform import (see "random_id.cache is empty tuple" terraform-google-modules/terraform-google-gcloud#66).

... Which is why I've opened a PR with a pretty limited "error_tag" hack that addresses the issue with hopefully no side-effects or wide reaching concerns.

@scottwinkler
Copy link
Owner

i now print env in the output, which should be enough to be able to uniquely identify resources. i will favor this over an error tag for now. let me know what you think, i can always add error tag later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants