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

Diagnostics Proposal #368

Merged
merged 19 commits into from
Apr 20, 2020
Merged

Diagnostics Proposal #368

merged 19 commits into from
Apr 20, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Mar 20, 2020

This PR implements diagnostics for validation, CRUD, and ConfigureContextFunc. There are breaking changes to some functions along the callstack but they are functions mostly considered "internal" despite being publicly exported.

Usage

Schema: map[string]*schema.Schema{
  "bar": {
    Type: schema.TypeMap,
    Required: true,
    ValidateDiagFunc: func(v interface{}, path cty.Path) diag.Diagnostics {
      // please build up a slice of diagnostics
      // the new signature passes you the cty.Path up to the attribute being validated
      // the SDK will automatically set the attribute path of all diagnostics to at least this point
      // therefore for primitive types, developers can leave the AttributePath unset
      // in the case of TypeMap, to have precise diagnostics, please provider either a relative path
      // or an absolute path to the key that has a diagnostic. 
      var diags diag.Diagnostics

      // set a relative path
      diags = append(diags, diag.Diagnostic{
        Severity: diag.Error,
        Summary: "error in map key",
        AttributePath: cty.Path{ cty.IndexStep{ Key: cty.StringVal("key_name") } },
      })

      // create the absolute path yourself with the passed in path
      diags = append(diags, diag.Diagnostic{
        Severity: diag.Error,
        Summary: "error in map key",
        AttributePath: append(path, cty.IndexStep{ Key: cty.StringVal("key_name")),
      })

      return diags
    }
  },
}

For diagnostics support in the other areas of the SDK (such as the context aware CRUD) the usage is similar except the developer is responsible for constructing and setting the path entirely (validation is unique in that the SDK drills down the schema)

TypeSet

Unfortunately schema.TypeSet cannot be represented as an attribute path with the current protocol. In the case of validation the path is truncated at that attribute, which will provide some context to the user but it is not perfect, err on the side of verbose wording for diagnostics that are a child of TypeSet.

We are still investigating having guardrails around this in the CRUD API so developers don't create invalid paths in general (relative to their schema definition). The situation isn't so bad though as Terraform will just render blank where the line and file info would have been (not great but also nonfatal)

diag/diagnostic.go Outdated Show resolved Hide resolved
diag/diagnostic.go Outdated Show resolved Hide resolved
diag/diagnostic.go Outdated Show resolved Hide resolved
diag/diagnostic.go Outdated Show resolved Hide resolved
diag/diagnostic.go Outdated Show resolved Hide resolved
diag/diagnostic.go Outdated Show resolved Hide resolved
diag/diagnostic.go Outdated Show resolved Hide resolved
@paultyng paultyng linked an issue Apr 3, 2020 that may be closed by this pull request
diag/path.go Outdated Show resolved Hide resolved
var ds []*proto.Diagnostic
for _, d := range diags {
if err := d.Validate(); err != nil {
panic(fmt.Errorf("Invalid diagnostic: %s. This is always a bug in the provider implementation", err))
Copy link
Contributor

Choose a reason for hiding this comment

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

why is this is an Errorf inside the panic? Should this func just return an error?

Copy link
Contributor Author

@appilon appilon Apr 8, 2020

Choose a reason for hiding this comment

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

I wanted to add the line that this should be considered a bug in the provider implementation, hence the interpolation/fmt. Wanted the errors coming from the diagnostic validation to be terse

diag/diagnostic.go Outdated Show resolved Hide resolved
helper/schema/resource.go Outdated Show resolved Hide resolved
@paultyng
Copy link
Contributor

Temporarily converted to draft while we work out the cty stuff, don't want to accidentally merge this.

build cty.Path as we dive through validation stack
make Error string of Diagnostic include detail
introduce MapKey to associate diag to proper attribute for maps during validation
@appilon appilon marked this pull request as ready for review April 17, 2020 01:09
@appilon appilon requested review from paultyng and kmoe April 17, 2020 01:10
Worth noting expecting strings for expected errors is brittle
should at least search for regex or substrings
update all internal use to diags.Append
Ensure both ValidateFunc and ValidateDiagFunc can't be set
truncate TypeSet attribute path and explain why
remove JoinPath helper
@ghost
Copy link

ghost commented May 21, 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 21, 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.

Support 0.12 Diagnostics in Providers
3 participants