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

Create diag package and switch all applicable []tfprotov6.Diagnostic usage #110

Merged
merged 14 commits into from
Sep 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changelog/110.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```release-note:breaking-change
Most uses of `[]*tfprotov6.Diagnostic` have been replaced with a new `diag.Diagnostics` type. Please update your type signatures, and use one of the `diags.New*` helper functions instead of constructing `*tfprotov6.Diagnostic`s by hand.
```

```release-note:feature
Introduced first-class diagnostics (`diag` package).
```
6 changes: 2 additions & 4 deletions attr/type.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package attr
import (
"context"

"github.com/hashicorp/terraform-plugin-go/tfprotov6"
"github.com/hashicorp/terraform-plugin-framework/diag"
"github.com/hashicorp/terraform-plugin-go/tftypes"
)

Expand Down Expand Up @@ -80,9 +80,7 @@ type TypeWithValidate interface {
// being used to populate the Type. It is generally used to check the
// data format and ensure that it complies with the requirements of the
// Type.
//
// TODO: don't use tfprotov6.Diagnostic, use our type
Validate(context.Context, tftypes.Value) []*tfprotov6.Diagnostic
Validate(context.Context, tftypes.Value) diag.Diagnostics
}

// TypeWithPlaintextDescription extends the Type interface to include a
Expand Down
45 changes: 45 additions & 0 deletions diag/attribute_error_diagnostic.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package diag
paddycarver marked this conversation as resolved.
Show resolved Hide resolved

import (
"github.com/hashicorp/terraform-plugin-go/tftypes"
)

var _ DiagnosticWithPath = AttributeErrorDiagnostic{}

// AttributeErrorDiagnostic is a generic attribute diagnostic with error severity.
type AttributeErrorDiagnostic struct {
ErrorDiagnostic

path *tftypes.AttributePath
}

// Equal returns true if the other diagnostic is wholly equivalent.
func (d AttributeErrorDiagnostic) Equal(other Diagnostic) bool {
aed, ok := other.(AttributeErrorDiagnostic)

if !ok {
return false
}

if !aed.Path().Equal(d.Path()) {
return false
}

return aed.ErrorDiagnostic.Equal(d.ErrorDiagnostic)
}

// Path returns the diagnostic path.
func (d AttributeErrorDiagnostic) Path() *tftypes.AttributePath {
return d.path
}

// NewAttributeErrorDiagnostic returns a new error severity diagnostic with the given summary, detail, and path.
func NewAttributeErrorDiagnostic(path *tftypes.AttributePath, summary string, detail string) AttributeErrorDiagnostic {
return AttributeErrorDiagnostic{
ErrorDiagnostic: ErrorDiagnostic{
detail: detail,
summary: summary,
},
path: path,
}
}
45 changes: 45 additions & 0 deletions diag/attribute_warning_diagnostic.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
package diag

import (
"github.com/hashicorp/terraform-plugin-go/tftypes"
)

var _ DiagnosticWithPath = AttributeWarningDiagnostic{}

// AttributeErrorDiagnostic is a generic attribute diagnostic with warning severity.
type AttributeWarningDiagnostic struct {
WarningDiagnostic

path *tftypes.AttributePath
}

// Equal returns true if the other diagnostic is wholly equivalent.
func (d AttributeWarningDiagnostic) Equal(other Diagnostic) bool {
awd, ok := other.(AttributeWarningDiagnostic)

if !ok {
return false
}

if !awd.Path().Equal(d.Path()) {
return false
}

return awd.WarningDiagnostic.Equal(d.WarningDiagnostic)
}

// Path returns the diagnostic path.
func (d AttributeWarningDiagnostic) Path() *tftypes.AttributePath {
return d.path
}

// NewAttributeWarningDiagnostic returns a new warning severity diagnostic with the given summary, detail, and path.
func NewAttributeWarningDiagnostic(path *tftypes.AttributePath, summary string, detail string) AttributeWarningDiagnostic {
return AttributeWarningDiagnostic{
WarningDiagnostic: WarningDiagnostic{
detail: detail,
summary: summary,
},
path: path,
}
}
49 changes: 49 additions & 0 deletions diag/diagnostic.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
package diag

import (
"github.com/hashicorp/terraform-plugin-go/tftypes"
)

// Diagnostic is an interface for providing enhanced feedback.
//
// These are typically practitioner facing, however it is possible for
// functionality, such as validation, to use these to change behaviors or
// otherwise have these be manipulated or removed before being presented.
//
// See the ErrorDiagnostic and WarningDiagnostic concrete types for generic
// implementations.
type Diagnostic interface {
// Severity returns the desired level of feedback for the diagnostic.
Severity() Severity

// Summary is a short description for the diagnostic.
//
// Typically this is implemented as a title, such as "Invalid Resource Name",
// or single line sentence.
Summary() string

// Detail is a long description for the diagnostic.
//
// This should contain all relevant information about why the diagnostic
// was generated and if applicable, ways to prevent the diagnostic. It
// should generally be written and formatted for human consumption by
// practitioners or provider developers.
Detail() string

// Equal returns true if the other diagnostic is wholly equivalent.
Equal(Diagnostic) bool
}

// DiagnosticWithPath is a diagnostic associated with an attribute path.
//
// This attribute information is used to display contextual source configuration
// to practitioners.
type DiagnosticWithPath interface {
Diagnostic

// Path points to a specific value within an aggregate value.
//
// If present, this enables the display of source configuration context for
// supporting implementations such as Terraform CLI commands.
Path() *tftypes.AttributePath
}
31 changes: 31 additions & 0 deletions diag/diagnostic_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
package diag_test

import (
"github.com/hashicorp/terraform-plugin-framework/diag"
)

var _ diag.Diagnostic = invalidSeverityDiagnostic{}

type invalidSeverityDiagnostic struct{}

func (d invalidSeverityDiagnostic) Detail() string {
return "detail for invalid severity diagnostic"
}

func (d invalidSeverityDiagnostic) Equal(other diag.Diagnostic) bool {
isd, ok := other.(invalidSeverityDiagnostic)

if !ok {
return false
}

return isd.Summary() == d.Summary() && isd.Detail() == d.Detail() && isd.Severity() == d.Severity()
}

func (d invalidSeverityDiagnostic) Severity() diag.Severity {
return diag.SeverityInvalid
}

func (d invalidSeverityDiagnostic) Summary() string {
return "summary for invalid severity diagnostic"
}
97 changes: 97 additions & 0 deletions diag/diagnostics.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
package diag

import (
"github.com/hashicorp/terraform-plugin-go/tfprotov6"
"github.com/hashicorp/terraform-plugin-go/tftypes"
)

// Diagnostics represents a collection of diagnostics.
//
// While this collection is ordered, the order is not guaranteed as reliable
// or consistent.
type Diagnostics []Diagnostic

// AddAttributeError adds a generic attribute error diagnostic to the collection.
func (diags *Diagnostics) AddAttributeError(path *tftypes.AttributePath, summary string, detail string) {
diags.Append(NewAttributeErrorDiagnostic(path, summary, detail))
}

// AddAttributeWarning adds a generic attribute warning diagnostic to the collection.
func (diags *Diagnostics) AddAttributeWarning(path *tftypes.AttributePath, summary string, detail string) {
diags.Append(NewAttributeWarningDiagnostic(path, summary, detail))
}

// AddError adds a generic error diagnostic to the collection.
func (diags *Diagnostics) AddError(summary string, detail string) {
diags.Append(NewErrorDiagnostic(summary, detail))
}

// AddWarning adds a generic warning diagnostic to the collection.
func (diags *Diagnostics) AddWarning(summary string, detail string) {
diags.Append(NewWarningDiagnostic(summary, detail))
}

// Append adds non-empty and non-duplicate diagnostics to the collection.
func (diags *Diagnostics) Append(in ...Diagnostic) {
bflad marked this conversation as resolved.
Show resolved Hide resolved
for _, diag := range in {
if diag == nil {
continue
}

if diags.Contains(diag) {
continue
}

if diags == nil {
*diags = Diagnostics{diag}
} else {
*diags = append(*diags, diag)
}
}
}

// Contains returns true if the collection contains an equal Diagnostic.
func (diags Diagnostics) Contains(in Diagnostic) bool {
for _, diag := range diags {
if diag.Equal(in) {
return true
}
}

return false
}

// HasError returns true if the collection has an error severity Diagnostic.
func (diags Diagnostics) HasError() bool {
for _, diag := range diags {
if diag.Severity() == SeverityError {
return true
}
}

return false
}

// ToTfprotov6Diagnostics converts the diagnostics into the tfprotov6 collection type.
//
// Usage of this method outside the framework is not supported nor considered
// for backwards compatibility promises.
func (diags Diagnostics) ToTfprotov6Diagnostics() []*tfprotov6.Diagnostic {
Copy link
Contributor

Choose a reason for hiding this comment

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

two things:

  1. This doesn't appear to require any privileged access to the diag package or benefit from being a method on the Diagnostics type. Is there a reason we shouldn't just make it a function in tfsdk?
  2. I think Tfprotov6 in this may be unnecessarily verbose. ToProto6Diagnostics is probably sufficient; we know which protocol we're talking about, and we're unlikely to be talking about any other protocol's diagnostics except Terraform's.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This doesn't appear to require any privileged access to the diag package or benefit from being a method on the Diagnostics type. Is there a reason we shouldn't just make it a function in tfsdk?

It does keep the diagnostics implementation altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm. Is that worth having an exported method we don't want people to call? 🤔 (I don't know the answer, I'm wondering out loud.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option here would be to move that exported function into a separate internal package, maybe along the lines of diag/internal/convert. I'd be more apt to do this so at least the code is tangential the other diagnostics code. I'd be worried about import cycles if we tried to setup something more generic like internal/toproto6.

@kmoe do you have any opinions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd still prefer this to be unexported as we don't expect provider developers to use it, and I'm worried about what they'll do with this unintended compatibility interface, but I'm not gonna hold this up over it.

var results []*tfprotov6.Diagnostic

for _, diag := range diags {
tfprotov6Diagnostic := &tfprotov6.Diagnostic{
Detail: diag.Detail(),
Severity: diag.Severity().ToTfprotov6DiagnosticSeverity(),
Summary: diag.Summary(),
}

if diagWithPath, ok := diag.(DiagnosticWithPath); ok {
tfprotov6Diagnostic.Attribute = diagWithPath.Path()
}

results = append(results, tfprotov6Diagnostic)
}

return results
}
Loading