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.

Sign up for GitHub

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

Update diag convenience methods #449

Merged
merged 1 commit into from
May 7, 2020
Merged

Update diag convenience methods #449

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 0 additions & 8 deletions diag/diagnostic.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,14 +93,6 @@ func (d Diagnostic) Validate() error {
return nil
}

// FromErr will convert an error into a Diagnostic
func FromErr(err error) Diagnostic {
return Diagnostic{
Severity: Error,
Summary: err.Error(),
}
}

// Severity is an enum type marking the severity level of a Diagnostic
type Severity int

Expand Down
36 changes: 36 additions & 0 deletions diag/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package diag

import "fmt"

// FromErr will convert an error into a Diagnostics. This returns Diagnostics
// as the most common use case in Go will be handling a single error
// returned from a function.
//
// if err != nil {
// return diag.FromErr(err)
// }
func FromErr(err error) Diagnostics {
Copy link
Contributor

@appilon appilon May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Overall I do like this pattern, however I would still like a convenience to create a singular Diagnostic from an error. Would it be crazy to keep the helpers for creating on a single diag, but then have a helper to quickly convert to the collection?

return diag.FromErr(err).ToDiags()

Copy link
Contributor

@appilon appilon May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

It's valuable in scenarios where we have gathered warnings, and encounter a function that just returns a go error

if err != nil {
    return append(diags, diag.FromErr(err))
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the thing is, that you are burdening the 80% case for the 20% case, and you could easily rewrite that second one as:

if err != nil {
    return append(diags, diag.FromErr(err)...)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I wish the package was called diags with this model, but that would collide with calling your slice diags.

return Diagnostics{
Diagnostic{
Severity: Error,
Summary: err.Error(),
},
}
}

// Errorf creates a Diagnostics with a single Error level Diagnostic entry.
// The summary is populated by performing a fmt.Sprintf with the supplied
// values. This returns a single error in a Diagnostics as errors typically
// do not occur in multiples as warnings may.
//
// if unexpectedCondition {
// return diag.Errorf("unexpected: %s", someValue)
// }
func Errorf(format string, a ...interface{}) Diagnostics {
return Diagnostics{
Diagnostic{
Severity: Error,
Summary: fmt.Sprintf(format, a...),
},
}
}
16 changes: 6 additions & 10 deletions helper/schema/provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,12 +243,9 @@ func (p *Provider) ValidateResource(
//
// This won't be called at all if no provider configuration is given.
func (p *Provider) Configure(ctx context.Context, c *terraform.ResourceConfig) diag.Diagnostics {

var diags diag.Diagnostics

// No configuration
if p.ConfigureFunc == nil && p.ConfigureContextFunc == nil {
return diags
return nil
Copy link
Contributor Author

@paultyng paultyng May 6, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I swapped some of these back, I think returning nil when possible is more clear that its not a potential error case. Preserving the go idioms if possible is still useful. When someone does have warnings or are propagating from diag aware functions, the top level diags and appends make sense, but otherwise, I think it may be more effort and compromises on clarity.

}

sm := schemaMap(p.Schema)
Expand All @@ -257,31 +254,30 @@ func (p *Provider) Configure(ctx context.Context, c *terraform.ResourceConfig) d
// generate an intermediary "diff" although that is never exposed.
diff, err := sm.Diff(ctx, nil, c, nil, p.meta, true)
if err != nil {
return append(diags, diag.FromErr(err))
return diag.FromErr(err)
}

data, err := sm.Data(nil, diff)
if err != nil {
return append(diags, diag.FromErr(err))
return diag.FromErr(err)
}

if p.ConfigureFunc != nil {
meta, err := p.ConfigureFunc(data)
if err != nil {
return append(diags, diag.FromErr(err))
return diag.FromErr(err)
}
p.meta = meta
}
if p.ConfigureContextFunc != nil {
meta, ds := p.ConfigureContextFunc(ctx, data)
diags = append(diags, ds...)
meta, diags := p.ConfigureContextFunc(ctx, data)
if diags.HasError() {
return diags
}
p.meta = meta
}

return diags
return nil
}

// Resources returns all the available resource types that this provider
Expand Down
2 changes: 1 addition & 1 deletion helper/schema/provider_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ func TestProviderConfigure(t *testing.T) {
return nil, nil
}

return nil, diag.Diagnostics{diag.FromErr(fmt.Errorf("nope"))}
return nil, diag.Errorf("nope")
},
},
Config: map[string]interface{}{
Expand Down
54 changes: 20 additions & 34 deletions helper/schema/resource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,11 +266,10 @@ type CustomizeDiffFunc func(context.Context, *ResourceDiff, interface{}) error

func (r *Resource) create(ctx context.Context, d *ResourceData, meta interface{}) diag.Diagnostics {
if r.Create != nil {
var diags diag.Diagnostics
if err := r.Create(d, meta); err != nil {
diags = append(diags, diag.FromErr(err))
return diag.FromErr(err)
}
return diags
return nil
}
ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutCreate))
defer cancel()
Expand All @@ -279,11 +278,10 @@ func (r *Resource) create(ctx context.Context, d *ResourceData, meta interface{}

func (r *Resource) read(ctx context.Context, d *ResourceData, meta interface{}) diag.Diagnostics {
if r.Read != nil {
var diags diag.Diagnostics
if err := r.Read(d, meta); err != nil {
diags = append(diags, diag.FromErr(err))
return diag.FromErr(err)
}
return diags
return nil
}
ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutRead))
defer cancel()
Expand All @@ -292,11 +290,10 @@ func (r *Resource) read(ctx context.Context, d *ResourceData, meta interface{})

func (r *Resource) update(ctx context.Context, d *ResourceData, meta interface{}) diag.Diagnostics {
if r.Update != nil {
var diags diag.Diagnostics
if err := r.Update(d, meta); err != nil {
diags = append(diags, diag.FromErr(err))
return diag.FromErr(err)
}
return diags
return nil
}
ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutUpdate))
defer cancel()
Expand All @@ -305,11 +302,10 @@ func (r *Resource) update(ctx context.Context, d *ResourceData, meta interface{}

func (r *Resource) delete(ctx context.Context, d *ResourceData, meta interface{}) diag.Diagnostics {
if r.Delete != nil {
var diags diag.Diagnostics
if err := r.Delete(d, meta); err != nil {
diags = append(diags, diag.FromErr(err))
return diag.FromErr(err)
}
return diags
return nil
}
ctx, cancel := context.WithTimeout(ctx, d.Timeout(TimeoutDelete))
defer cancel()
Expand All @@ -322,12 +318,9 @@ func (r *Resource) Apply(
s *terraform.InstanceState,
d *terraform.InstanceDiff,
meta interface{}) (*terraform.InstanceState, diag.Diagnostics) {

var diags diag.Diagnostics

data, err := schemaMap(r.Schema).Data(s, d)
if err != nil {
return s, append(diags, diag.FromErr(err))
return s, diag.FromErr(err)
}

if s != nil && data != nil {
Expand Down Expand Up @@ -358,11 +351,12 @@ func (r *Resource) Apply(
s = new(terraform.InstanceState)
}

var diags diag.Diagnostics

if d.Destroy || d.RequiresNew() {
if s.ID != "" {
// Destroy the resource since it is created
diags = append(diags, r.delete(ctx, data, meta)...)

if diags.HasError() {
return r.recordCurrentSchemaVersion(data.State()), diags
}
Expand All @@ -380,7 +374,7 @@ func (r *Resource) Apply(
// Reset the data to be stateless since we just destroyed
data, err = schemaMap(r.Schema).Data(nil, d)
if err != nil {
return nil, append(diags, diag.FromErr(err))
return nil, append(diags, diag.FromErr(err)...)
}

// data was reset, need to re-apply the parsed timeouts
Expand Down Expand Up @@ -485,18 +479,14 @@ func (r *Resource) ReadDataApply(
d *terraform.InstanceDiff,
meta interface{},
) (*terraform.InstanceState, diag.Diagnostics) {

var diags diag.Diagnostics

// Data sources are always built completely from scratch
// on each read, so the source state is always nil.
data, err := schemaMap(r.Schema).Data(nil, d)
if err != nil {
return nil, append(diags, diag.FromErr(err))
return nil, diag.FromErr(err)
}

diags = append(diags, r.read(ctx, data, meta)...)

diags := r.read(ctx, data, meta)
state := data.State()
if state != nil && state.ID == "" {
// Data sources can set an ID if they want, but they aren't
Expand All @@ -517,12 +507,9 @@ func (r *Resource) RefreshWithoutUpgrade(
ctx context.Context,
s *terraform.InstanceState,
meta interface{}) (*terraform.InstanceState, diag.Diagnostics) {

var diags diag.Diagnostics

// If the ID is already somehow blank, it doesn't exist
if s.ID == "" {
return nil, diags
return nil, nil
}

rt := ResourceTimeout{}
Expand All @@ -537,7 +524,7 @@ func (r *Resource) RefreshWithoutUpgrade(
// affect our Read later.
data, err := schemaMap(r.Schema).Data(s, nil)
if err != nil {
return s, append(diags, diag.FromErr(err))
return s, diag.FromErr(err)
}
data.timeouts = &rt

Expand All @@ -547,26 +534,25 @@ func (r *Resource) RefreshWithoutUpgrade(

exists, err := r.Exists(data, meta)
if err != nil {
return s, append(diags, diag.FromErr(err))
return s, diag.FromErr(err)
}

if !exists {
return nil, diags
return nil, nil
}
}

data, err := schemaMap(r.Schema).Data(s, nil)
if err != nil {
return s, append(diags, diag.FromErr(err))
return s, diag.FromErr(err)
}
data.timeouts = &rt

if s != nil {
data.providerMeta = s.ProviderMeta
}

diags = append(diags, r.read(ctx, data, meta)...)

diags := r.read(ctx, data, meta)
state := data.State()
if state != nil && state.ID == "" {
state = nil
Expand Down