-
Notifications
You must be signed in to change notification settings - Fork 99
Add ListResource RPC #1157
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 ListResource RPC #1157
Changes from 2 commits
d07f61a
ce0467f
fe9a7fa
a193b8d
a5a3d3f
3ff5bfa
eecfd76
7f3191d
f6b4307
38660df
d18edd0
7e41955
9590b11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,145 @@ | ||
// Copyright (c) HashiCorp, Inc. | ||
// SPDX-License-Identifier: MPL-2.0 | ||
|
||
package fwserver | ||
|
||
import ( | ||
"context" | ||
"errors" | ||
"iter" | ||
|
||
"github.com/hashicorp/terraform-plugin-framework/diag" | ||
"github.com/hashicorp/terraform-plugin-framework/internal/fwschema" | ||
"github.com/hashicorp/terraform-plugin-framework/internal/logging" | ||
"github.com/hashicorp/terraform-plugin-framework/list" | ||
"github.com/hashicorp/terraform-plugin-framework/tfsdk" | ||
) | ||
|
||
// ListRequest is the framework server request for the ListResource RPC. | ||
type ListRequest struct { | ||
// ListResource is an instance of the provider's list resource | ||
// implementation for a specific managed resource type. | ||
ListResource list.ListResource | ||
|
||
// Config is the configuration the user supplied for listing resource | ||
// instances. | ||
Config *tfsdk.Config | ||
|
||
// IncludeResource indicates whether the provider should populate the | ||
// Resource field in the ListResult struct. | ||
IncludeResource bool | ||
|
||
ResourceSchema fwschema.Schema | ||
ResourceIdentitySchema fwschema.Schema | ||
} | ||
|
||
// ListResultsStream represents a streaming response to a [ListRequest]. An | ||
// instance of this struct is supplied as an argument to the provider's List | ||
// function. The provider should set a Results iterator function that pushes | ||
// zero or more results of type [ListResult]. | ||
// | ||
// For convenience, a provider implementation may choose to convert a slice of | ||
// results into an iterator using [slices.Values]. | ||
type ListResultsStream struct { | ||
// Results is a function that emits [ListResult] values via its push | ||
// function argument. | ||
Results iter.Seq[ListResult] | ||
} | ||
|
||
func ListResultError(summary string, detail string) ListResult { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TODO: add a doc comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's not a "publicly" exported field, no need to add a doc comment unless you feel this helper needs more explanation for maintainers. Although I'm wondering if this function even needs to be exported (or even exist, it's only used once ATM, maybe it will be used more later?) |
||
return ListResult{ | ||
Diagnostics: diag.Diagnostics{ | ||
diag.NewErrorDiagnostic(summary, detail), | ||
}, | ||
} | ||
} | ||
|
||
// ListResult represents a listed managed resource instance. | ||
type ListResult struct { | ||
// Identity is the identity of the managed resource instance. A nil value | ||
// will raise will raise a diagnostic. | ||
bbasata marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Identity *tfsdk.ResourceIdentity | ||
|
||
// Resource is the provider's representation of the attributes of the | ||
// listed managed resource instance. | ||
// | ||
// If [ListRequest.IncludeResource] is true, a nil value will raise | ||
// a warning diagnostic. | ||
Resource *tfsdk.Resource | ||
|
||
// DisplayName is a provider-defined human-readable description of the | ||
// listed managed resource instance, intended for CLI and browser UIs. | ||
DisplayName string | ||
|
||
// Diagnostics report errors or warnings related to the listed managed | ||
// resource instance. An empty slice indicates a successful operation with | ||
// no warnings or errors generated. | ||
Diagnostics diag.Diagnostics | ||
} | ||
|
||
// ListResource implements the framework server ListResource RPC. | ||
func (s *Server) ListResource(ctx context.Context, fwReq *ListRequest, fwStream *ListResultsStream) error { | ||
listResource := fwReq.ListResource | ||
|
||
if fwReq.Config == nil { | ||
return errors.New("Invalid ListResource request: Config cannot be nil") | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason we aren't using response diagnostics here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no functional reason. I was on the fence re: Now that we have a 👍 on "push a single event with an error diagnostic," we could totally use diagnostics instead. I'm open to that. Any preference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of framework (like the type system for example) uses diagnostics, which are just a summary/detail, so I'd say we should stick to diagnostics unless we can't for whatever reason 👍🏻 |
||
|
||
req := list.ListRequest{ | ||
Config: *fwReq.Config, | ||
IncludeResource: fwReq.IncludeResource, | ||
ResourceSchema: fwReq.ResourceSchema, // TODO: revisit | ||
ResourceIdentitySchema: fwReq.ResourceIdentitySchema, // TODO: revisit | ||
} | ||
|
||
stream := &list.ListResultsStream{} | ||
|
||
logging.FrameworkTrace(ctx, "Calling provider defined ListResource") | ||
listResource.List(ctx, req, stream) | ||
logging.FrameworkTrace(ctx, "Called provider defined ListResource") | ||
|
||
// If the provider returned a nil results stream, we return an empty stream. | ||
if stream.Results == nil { | ||
stream.Results = list.NoListResults | ||
} | ||
|
||
fwStream.Results = processListResults(req, stream.Results) | ||
return nil | ||
} | ||
|
||
func processListResults(req list.ListRequest, stream iter.Seq[list.ListResult]) iter.Seq[ListResult] { | ||
return func(push func(ListResult) bool) { | ||
for result := range stream { | ||
if !push(processListResult(req, result)) { | ||
return | ||
} | ||
} | ||
} | ||
} | ||
|
||
// processListResult validates the content of a list.ListResult and returns a | ||
// ListResult | ||
func processListResult(req list.ListRequest, result list.ListResult) ListResult { | ||
if result.Diagnostics.HasError() { | ||
return ListResult(result) | ||
} | ||
|
||
if result.Identity == nil { // TODO: is result.Identity.Raw.IsNull() a practical concern? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess the question would: does Terraform core care if the provider returns a In which case we should just check both:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Terraform currently raises an error when a list result has no identity |
||
return ListResultError( | ||
"Incomplete List Result", | ||
"The provider did not populate the Identity field in the ListResourceResult. This may be due to an error in the provider's implementation.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: We can probably use more guided language since it's almost certainly a bug in the provider and should be reported there first: Something to the tune of -> There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. +1. Updating. |
||
) | ||
} | ||
|
||
if req.IncludeResource { | ||
if result.Resource == nil { // TODO: is result.Resource.Raw.IsNull() a practical concern? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as the above comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitHub notifications aren't working well today, I hope I don't miss any mentions. Ideally a provider should always send a resource when include resource is true, but Terraform doesn't enforce that |
||
result.Resource = nil | ||
bbasata marked this conversation as resolved.
Show resolved
Hide resolved
|
||
result.Diagnostics.AddWarning( | ||
"Incomplete List Result", | ||
"The provider did not populate the Resource field in the ListResourceResult. This may be due to the provider not supporting this functionality or an error in the provider's implementation.", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps future PR, but if this is a warning that practitioners are likely to see often, maybe we should tailor it specifically to them (i.e. not using the Go types since they have no reference of what these fields mean and won't be reporting to provider developers) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree, and +1 to a future PR for this. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💭 I'm less certain of the guidance for a nil Resource warning. It's a warning, after all. Is it actionable by the user? Open to suggestions. |
||
) | ||
} | ||
} | ||
|
||
return ListResult(result) // TODO: do we need to .Copy() the raw Identity and Resource values? | ||
} |
Uh oh!
There was an error while loading. Please reload this page.