feat(blocks): add update resource operation #198

mitchnielsen · 2024-06-06T21:45:57Z

Summary

Closes #182.

Testing (click to expand)

Testing - OSS

Setup

First, run docker-compose up -d to start a local Prefect instance.

Next, create a block resource to use with the local instance, and enable the provider:

diff --git a/examples/provider-install-verification/main.tf b/examples/provider-install-verification/main.tf
index 7799578..f1119ee 100644
--- a/examples/provider-install-verification/main.tf
+++ b/examples/provider-install-verification/main.tf
@@ -10,9 +10,14 @@ provider "prefect" {
   endpoint = "http://localhost:4200/api"
 }
 
-resource "prefect_work_pool" "example" {
-  name        = "my-work-pool"
-  type        = "kubernetes"
-  description = "example work pool"
-  paused      = true
+resource "prefect_block" "secret" {
+  name      = "foo"
+  type_slug = "secret"
+
+  data = jsonencode({
+    "value" : "bar"
+  })
+
+  # set the workpace_id attribute on the provider OR the resource
+  # workspace_id = "<workspace UUID>"
 }
diff --git a/internal/provider/provider.go b/internal/provider/provider.go
index f7f4ef4..5f95b51 100644
--- a/internal/provider/provider.go
+++ b/internal/provider/provider.go
@@ -242,6 +242,6 @@ func (p *PrefectProvider) Resources(_ context.Context) []func() resource.Resourc
 		resources.NewWorkspaceAccessResource,
 		resources.NewWorkspaceResource,
 		resources.NewWorkspaceRoleResource,
-		// resources.NewBlockResource,
+		resources.NewBlockResource,
 	}
 }

Apply it with Terraform:

Terraform will perform the following actions:

  # prefect_block.secret will be created
  + resource "prefect_block" "secret" {
      + created   = (known after apply)
      + data      = jsonencode(
            {
              + value = "bar"
            }
        )
      + id        = (known after apply)
      + name      = "foo"
      + type_slug = "secret"
      + updated   = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

prefect_block.secret: Creating...
prefect_block.secret: Creation complete after 0s [id=5e1f535d-ff3c-4164-9117-e1ecba53a122]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Now that it's created, try to read the value. We can use a quick Python script in test.py:

from prefect.blocks.system import Secret

print(Secret.load("foo").get())

$ python test.py
bar

Now let's change it so we can test Update. Change the value in main.tf to baz.

Next, run terraform plan:

prefect_block.secret: Refreshing state... [id=5e1f535d-ff3c-4164-9117-e1ecba53a122]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # prefect_block.secret will be updated in-place
  ~ resource "prefect_block" "secret" {
      ~ created   = "2024-06-06T21:05:27Z" -> (known after apply)
      ~ data      = jsonencode(
          ~ {
              ~ value = "bar" -> "baz"
            }
        )
        id        = "5e1f535d-ff3c-4164-9117-e1ecba53a122"
        name      = "foo"
      ~ updated   = "2024-06-06T21:05:27Z" -> (known after apply)
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Apply the change with terraform apply and then check the value again:

$ python test.py
baz

The value was successfully updated.

Testing - Cloud

First, update the main.tf file to point to Staging and reset the block value to bar:

diff --git a/examples/provider-install-verification/main.tf b/examples/provider-install-verification/main.tf
index 7799578..55db778 100644
--- a/examples/provider-install-verification/main.tf
+++ b/examples/provider-install-verification/main.tf
@@ -7,12 +7,19 @@ terraform {
 }
 
 provider "prefect" {
-  endpoint = "http://localhost:4200/api"
+  endpoint     = "https://api.prefect.cloud"
+  account_id   = "<my account ID>"
+  workspace_id = "<my workspace ID>"
 }
 
-resource "prefect_work_pool" "example" {
-  name        = "my-work-pool"
-  type        = "kubernetes"
-  description = "example work pool"
-  paused      = true
+resource "prefect_block" "secret" {
+  name      = "foo"
+  type_slug = "secret"
+
+  data = jsonencode({
+    "value" : "bar"
+  })
+
+  # set the workpace_id attribute on the provider OR the resource
+  # workspace_id = "<workspace UUID>"
 }

Apply it with Terraform:

$ terraform plan
...

Terraform will perform the following actions:

  # prefect_block.secret will be created
  + resource "prefect_block" "secret" {
      + created   = (known after apply)
      + data      = jsonencode(
            {
              + value = "bar"
            }
        )
      + id        = (known after apply)
      + name      = "foo"
      + type_slug = "secret"
      + updated   = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

prefect_block.secret: Creating...
prefect_block.secret: Creation complete after 0s [id=d8ae1c60-1248-407b-9ad8-ddb412ac6b1a]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Let's test it with Python:

$ python test.py
bar

And let's change it to baz in main.tf and apply it to confirm Update:

$ terraform apply
...
Terraform will perform the following actions:

  # prefect_block.secret will be updated in-place
  ~ resource "prefect_block" "secret" {
      ~ created   = "2024-06-06T21:40:55Z" -> (known after apply)
      ~ data      = jsonencode(
          ~ {
              ~ value = "bar" -> "baz"
            }
        )
        id        = "4f308f8f-cb19-4823-aabc-bbd8debe4af1"
        name      = "foo"
      ~ updated   = "2024-06-06T21:40:55Z" -> (known after apply)
        # (1 unchanged attribute hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

prefect_block.secret: Modifying... [id=4f308f8f-cb19-4823-aabc-bbd8debe4af1]
prefect_block.secret: Modifications complete after 1s [id=4f308f8f-cb19-4823-aabc-bbd8debe4af1]

Apply complete! Resources: 0 added, 1 changed, 0 destroyed.

And get the new value with Python:

$ python test.py
baz

Implements Update method for BlockResource type. Closes #182

Catches two scenarios where we need to 'return' when err != nil.

mitchnielsen · 2024-06-06T22:09:10Z

There's a fair amount of repetition in internal/provider/resources/block.go, but to keep these PRs simple I'll just put this one up for review and we can DRY things up separately.

parkedwards · 2024-06-06T23:52:47Z

internal/provider/resources/block.go

+	blockTypeClient, err := r.client.BlockTypes(plan.AccountID.ValueUUID(), plan.WorkspaceID.ValueUUID())
+	if err != nil {
+		resp.Diagnostics.Append(helpers.CreateClientErrorDiagnostic("Block Types", err))
+
+		return
+	}
+
+	blockSchemaClient, err := r.client.BlockSchemas(plan.AccountID.ValueUUID(), plan.WorkspaceID.ValueUUID())
+	if err != nil {
+		resp.Diagnostics.Append(helpers.CreateClientErrorDiagnostic("Block Schema", err))
+
+		return
+	}
+
+	blockDocumentClient, err := r.client.BlockDocuments(plan.AccountID.ValueUUID(), plan.WorkspaceID.ValueUUID())
+	if err != nil {
+		resp.Diagnostics.Append(helpers.CreateClientErrorDiagnostic("Block Document", err))
+
+		return
+	}


wonder if we just make a BlockManager aggregator class that initializes the sub-clients and exposes a single set of methods that calls these underneath the hood

Yeah, I started abstracting a helper method to get the clients but then the PR started getting bigger since I was modifying other methods. Let me make an issue for that.

parkedwards · 2024-06-07T00:00:12Z

internal/provider/resources/block.go

+		return
+	}
+
+	blockType, err := blockTypeClient.GetBySlug(ctx, plan.TypeSlug.ValueString())


ok here's an interesting thing that came up - so the PATCH endpoint accepts a block_schema_id (meaning it's a modifiable attribute). block_schemas are a "version" of a type

but what if the type_slug is modified in the TF resource? then we'll get a failure like so -

Terraform will perform the following actions: # prefect_block.lambda will be updated in-place ~ resource "prefect_block" "lambda" { id = "adf48aa0-318d-40d1-aca0-c897f05cc22b" name = "lambda" ~ type_slug = "lambda-function" -> "secret" ~ updated = "2024-06-06T23:40:59Z" -> (known after apply) # (3 unchanged attributes hidden) } Plan: 0 to add, 1 to change, 0 to destroy. prefect_block.lambda: Modifying... [id=adf48aa0-318d-40d1-aca0-c897f05cc22b] ╷ │ Error: Error during update Block Document │ │ with prefect_block.lambda, │ on main.tf line 19, in resource "prefect_block" "lambda": │ 19: resource "prefect_block" "lambda" { │ │ Could not update Block Document, unexpected error: status code 400 Bad Request, error={"detail":"Must migrate block document to a block schema of the same │ block type."}

we can find out, but im guessing the intent of the endpoint was to allow changing schema versions of the same block type, but we shouldnt' allow changing the type itself

so there's two things here:

should we prevent users from changing the type_slug? i think we should - but since the Schema object is shared, we may need to enforce this in this method (by comparing the plan.TypeSlug == state.TypeSlug => exit early if they do)

should we allow users modifying the schema of the same type? im not sure we need to (eg. the UI does not expose this). also since we take the "[0]" schema from blockSchemaClient.List, we're effectively applying the latest schema on every update call

should we prevent users from changing the type_slug? i think we should - but since the Schema object is shared, we may need to enforce this in this method (by comparing the plan.TypeSlug == state.TypeSlug => exit early if they do)

@parkedwards good call-out. I started looking into this but haven't yet found a way to work with state and plan in the same place 🤔 Am I missing something?

I found a reference to DiffSuppressFunc, which isn't what we want, but does allow you to work with old and new values to compare them.

I'll keep looking but if you know of a way off the top of your head just lemme know 👍🏼

Maybe RequiresReplaceIf could help here.

take a peek at the service_account.go resource's Update() method - i had to do some similar kinds of plan vs. state checks. the high level is that you can just invoke req.Plan.Get() or req.State.Get(), depending on what you want to draw from

https://github.com/PrefectHQ/terraform-provider-prefect/blob/main/internal/provider/resources/service_account.go#L331-L339

That change results in:

$ terraform apply ... Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: -/+ destroy and then create replacement Terraform will perform the following actions: # prefect_block.secret must be replaced -/+ resource "prefect_block" "secret" { ~ created = "2024-06-07T16:32:08Z" -> (known after apply) ~ id = "b015fc44-4a05-4f41-90b6-2d2523bb92bb" -> (known after apply) name = "foo" ~ type_slug = "secret" -> "lambda" # forces replacement ~ updated = "2024-06-07T16:32:08Z" -> (known after apply) # (1 unchanged attribute hidden) } Plan: 1 to add, 0 to change, 1 to destroy.

What do you think of that flow @parkedwards? Or would you rather reject the change entirely?

oh i completely missed your comment about using the planmodifier. yeah i like it!

the high level is that you can just invoke req.Plan.Get() or req.State.Get()

🤦🏼 I tried that and my editor didn't want to autocomplete .State so I assumed it wasn't available. Must have had an error elsewhere in my file. Thanks!

The planmodifier approach feels more "correct" here based on some googling but I'm also open to just grabbing State and Plan in the same place. Do you have a preference?

I went with planmodifier.RequiresReplace in 5995859 for now as as it was the least amount of code, and I saw we use it in 2 other places in the codebase already. If we ever want to change this functionality to instead reject the change and throw an error, we know where to look.

should we allow users modifying the schema of the same type? im not sure we need to (eg. the UI does not expose this). also since we take the "[0]" schema from blockSchemaClient.List, we're effectively applying the latest schema on every update call

Fair question, probably not for now if the UI doesn't expose that. Although maybe it's worth a debug log of the block schema's metadata 🤔

parkedwards

overall look awesome, i tested this a couple different ways in OSS and Cloud and everythign seems to check out 🚀

i would just look into that validation that prevents users from changing type_slug on an update

If the 'type_slug' attribute changes, we'll need to replace the entire resource because it's of a different type.

parkedwards · 2024-06-07T17:17:15Z

internal/provider/resources/block.go

@@ -106,6 +106,9 @@ func (r *BlockResource) Schema(_ context.Context, _ resource.SchemaRequest, resp
 			"type_slug": schema.StringAttribute{
 				Required:    true,
 				Description: "Block Type slug, which determines the schema of the `data` JSON attribute. Use `prefect block types ls` to view all available Block type slugs.",
+				PlanModifiers: []planmodifier.String{
+					stringplanmodifier.RequiresReplace(),


mitchnielsen added 2 commits June 6, 2024 15:59

Implement BlockResource.Update method

b193a66

Implements Update method for BlockResource type. Closes #182

Ensure we return when err is not nil

6c82d2d

Catches two scenarios where we need to 'return' when err != nil.

mitchnielsen added the enhancement New feature or request label Jun 6, 2024

mitchnielsen self-assigned this Jun 6, 2024

mitchnielsen temporarily deployed to Acceptance Tests June 6, 2024 21:46 — with GitHub Actions Inactive

mitchnielsen marked this pull request as ready for review June 6, 2024 22:09

mitchnielsen requested review from gabcoyne and a team as code owners June 6, 2024 22:09

parkedwards reviewed Jun 6, 2024

View reviewed changes

parkedwards reviewed Jun 7, 2024

View reviewed changes

Require replace if 'type_slug' attribute changes

5995859

If the 'type_slug' attribute changes, we'll need to replace the entire resource because it's of a different type.

mitchnielsen temporarily deployed to Acceptance Tests June 7, 2024 16:58 — with GitHub Actions Inactive

mitchnielsen mentioned this pull request Jun 7, 2024

Reduce repetition in codebase #203

Open

5 tasks

parkedwards approved these changes Jun 7, 2024

View reviewed changes

mitchnielsen merged commit 5a383a3 into main Jun 7, 2024
7 checks passed

mitchnielsen deleted the 182-block-update branch June 7, 2024 18:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(blocks): add update resource operation #198

feat(blocks): add update resource operation #198

mitchnielsen commented Jun 6, 2024 •

edited

Loading

mitchnielsen commented Jun 6, 2024

parkedwards Jun 6, 2024 •

edited

Loading

mitchnielsen Jun 7, 2024

mitchnielsen Jun 7, 2024

parkedwards Jun 7, 2024 •

edited

Loading

mitchnielsen Jun 7, 2024

mitchnielsen Jun 7, 2024

parkedwards Jun 7, 2024

mitchnielsen Jun 7, 2024

parkedwards Jun 7, 2024

mitchnielsen Jun 7, 2024

mitchnielsen Jun 7, 2024

mitchnielsen Jun 7, 2024

parkedwards left a comment

parkedwards Jun 7, 2024

feat(blocks): add update resource operation #198

feat(blocks): add update resource operation #198

Conversation

mitchnielsen commented Jun 6, 2024 • edited Loading

Summary

Testing - OSS

Setup

Testing - Cloud

mitchnielsen commented Jun 6, 2024

parkedwards Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkedwards Jun 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parkedwards left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchnielsen commented Jun 6, 2024 •

edited

Loading

parkedwards Jun 6, 2024 •

edited

Loading

parkedwards Jun 7, 2024 •

edited

Loading