add resource for datastream lifecycle#852
Conversation
tobio
left a comment
tobio
left a comment
There was a problem hiding this comment.
Thanks for taking the time to add this in.
Is it possible to have this resource built on the Plugin Framework instead? There's a strong preference here for new code to utilise that framework over the old SDK v2 and we're not re-using any existing SDK code here. This commit is an example of converting to the plugin framework if that helps.
It looks like lint is failing because the generated docs don't match what's committed, which can be fixed by committing the result of make docs-generate
| // generate renadom name | ||
| dsName := sdkacctest.RandStringFromCharSet(22, sdkacctest.CharSetAlpha) | ||
|
|
||
| resource.Test(t, resource.TestCase{ |
There was a problem hiding this comment.
We'll want to skip these when the backing cluster doesn't support DS lifecycles (example)
| ) | ||
|
|
||
| func TestAccResourceDataStreamLifecycle(t *testing.T) { | ||
| // generate renadom name |
There was a problem hiding this comment.
| // generate renadom name | |
| // generate random name |
|
|
||
| {{ tffile "examples/resources/elasticstack_elasticsearch_data_stream_lifecycle/resource.tf" }} | ||
|
|
||
| {{ .SchemaMarkdown | trimspace }} No newline at end of file |
| }, | ||
| }, | ||
| }, | ||
| "lifecycles": { |
There was a problem hiding this comment.
It's not clear what value we get with this block.
Since we're not writing these values back to the top level attributes TF drift detection won't pickup out of band changes. Choosing which value to use when the response includes multiple lifecycles is a challenge, but something we should solve IMO (a simple heuristic would be to use any value which differed from the configured value so drift detection worked, and if no diff was found just use the first).
In reality, I suspect most users will likely target a single data stream.
There was a problem hiding this comment.
Originally I had it so that I only detected the value that changed, then I thought it would be nice to have everything in state, but I didn't know that TF couldn't work with it that way.
I will change it back :)
ok, not a problem. |
aizerin
force-pushed
the
feature/ds_lifecycle
branch
2 times, most recently
from
20c1f15 to
fe15c35
Compare
|
updated |
tobio
left a comment
tobio
left a comment
There was a problem hiding this comment.
Thanks for adding this resource, it's really useful!
One small comment on de-duplicating the Create/Update implementations but otherwise LGTM.
| "github.com/hashicorp/terraform-plugin-framework/types" | ||
| ) | ||
|
|
||
| func (r *Resource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) { |
There was a problem hiding this comment.
This looks identical to the Create method. Can we pull this out to a common write (or some other name if you prefer) method and re-use it in both Create and Update?
There was a problem hiding this comment.
OK, I did the same as here:
internal/fleet/integration/create.go
internal/fleet/integration/update.go
aizerin
force-pushed
the
feature/ds_lifecycle
branch
from
fe15c35 to
32987b8
Compare
tobio
left a comment
tobio
left a comment
There was a problem hiding this comment.
Love it, thanks again for taking the time to get this one added, and for working through the review changes.
* add resource for datastream lifecycle * Just return diags --------- Co-authored-by: Toby Brain <toby.brain@elastic.co>
2 participants
resolves #838