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

feat(blocks): add update resource operation #198

Merged
merged 3 commits into from
Jun 7, 2024
Merged

feat(blocks): add update resource operation #198

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b193a66
Implement BlockResource.Update method
mitchnielsen Jun 6, 2024
6c82d2d
Ensure we return when err is not nil
mitchnielsen Jun 6, 2024
5995859
Require replace if 'type_slug' attribute changes
mitchnielsen Jun 7, 2024
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
2 changes: 1 addition & 1 deletion internal/api/block_documents.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type BlockDocumentCreate struct {
}

type BlockDocumentUpdate struct {
BlockSchemaID *uuid.UUID `json:"block_schema_id"`
BlockSchemaID uuid.UUID `json:"block_schema_id"`
Data map[string]interface{} `json:"data"`
MergeExistingData bool `json:"merge_existing_data"`
}
Expand Down
121 changes: 119 additions & 2 deletions internal/provider/resources/block.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

},
},
"data": schema.StringAttribute{
Required: true,
Expand Down Expand Up @@ -256,6 +259,8 @@ func (r *BlockResource) Read(ctx context.Context, req resource.ReadRequest, resp
"Error creating block client",
fmt.Sprintf("Could not create block client, unexpected error: %s. This is a bug in the provider, please report this to the maintainers.", err.Error()),
)

return
}

var blockID uuid.UUID
Expand Down Expand Up @@ -306,9 +311,121 @@ func (r *BlockResource) Read(ctx context.Context, req resource.ReadRequest, resp
}

// Update updates the resource and sets the updated Terraform state on success.
//
//nolint:revive // TODO: remove this comment when method is implemented
func (r *BlockResource) Update(ctx context.Context, req resource.UpdateRequest, resp *resource.UpdateResponse) {
var plan BlockResourceModel
resp.Diagnostics.Append(req.Plan.Get(ctx, &plan)...)
if resp.Diagnostics.HasError() {
return
}

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
}
Comment on lines +321 to +340
Copy link
Contributor

@parkedwards parkedwards Jun 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#203


blockType, err := blockTypeClient.GetBySlug(ctx, plan.TypeSlug.ValueString())
Copy link
Contributor

@parkedwards parkedwards Jun 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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:

  1. 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)
  2. 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe RequiresReplaceIf could help here.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

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 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🤔

if err != nil {
resp.Diagnostics.Append(helpers.ResourceClientErrorDiagnostic("Block Type", "get_by_slug", err))

return
}

blockSchemas, err := blockSchemaClient.List(ctx, []uuid.UUID{blockType.ID})
if err != nil {
resp.Diagnostics.Append(helpers.ResourceClientErrorDiagnostic("Block Schema", "list", err))

return
}

if len(blockSchemas) == 0 {
resp.Diagnostics.AddError(
"No block schemas found",
fmt.Sprintf("No block schemas found for %s block type slug", plan.TypeSlug.ValueString()),
)

return
}

latestBlockSchema := blockSchemas[0]

blockID, err := uuid.Parse(plan.ID.ValueString())
if err != nil {
resp.Diagnostics.AddAttributeError(
path.Root("id"),
"Error parsing block ID",
fmt.Sprintf("Could not parse block ID to UUID, unexpected error: %s", err.Error()),
)

return
}

var data map[string]interface{}
resp.Diagnostics.Append(plan.Data.Unmarshal(&data)...)
if resp.Diagnostics.HasError() {
return
}

err = blockDocumentClient.Update(ctx, blockID, api.BlockDocumentUpdate{
BlockSchemaID: latestBlockSchema.ID,
Data: data,
MergeExistingData: true,
})

if err != nil {
resp.Diagnostics.Append(helpers.ResourceClientErrorDiagnostic("Block Document", "update", err))

return
}

block, err := blockDocumentClient.Get(ctx, blockID)
if err != nil {
resp.Diagnostics.AddError(
"Error refreshing block state",
fmt.Sprintf("Could not read block, unexpected error: %s", err.Error()),
)

return
}

diags := copyBlockToModel(block, &plan)
resp.Diagnostics.Append(diags...)
if resp.Diagnostics.HasError() {
return
}

byteSlice, err := json.Marshal(block.Data)
if err != nil {
diags.AddAttributeError(
path.Root("data"),
"Failed to serialize Block Data",
fmt.Sprintf("Could not serialize Block Data as JSON string: %s", err.Error()),
)

return
}
plan.Data = jsontypes.NewNormalizedValue(string(byteSlice))

diags = resp.State.Set(ctx, &plan)
resp.Diagnostics.Append(diags...)
if resp.Diagnostics.HasError() {
return
}
}

// Delete deletes the resource and removes the Terraform state on success.
Expand Down
Loading