Skip to content

Commit

Permalink
fix(resources)!: publish_pipeline should be re-created every time
Browse files Browse the repository at this point in the history
Selectively publishing a pipeline doesn't really work in Terraform due
to race conditions. The pipeline resources are updated atomically
outside of the scope of `publish_pipeline`, therefore it's possible that
states will update even if the publish command fails (paused pipeline,
for example). This causes a situation where no changes are detected, and
thus `publish_pipeline` does not become part of the `plan`.

To fix this, constantly remove the `publish_pipeline` from state every
time `Read()` is called. This will force "re-creation" of the publish
resource such that it will be executed every time. Since the
`ENOCHANGES` error is ignored, this operation becomes idempotent.

BREAKING CHANGE: This removes the required `updated_at` property from
the schema. Users will get an error if `updated_at` is required, so this
is a breaking change.

Ref: LOG-20085
  • Loading branch information
darinspivey committed Jun 10, 2024
1 parent 79b713d commit 41957c8
Show file tree
Hide file tree
Showing 6 changed files with 12 additions and 39 deletions.
2 changes: 0 additions & 2 deletions docs/resources/publish_pipeline.md
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ module "pipeline" {
resource "mezmo_publish_pipeline" "publisher" {
pipeline_id = module.pipeline.my_pipeline.id
updated_at = module.pipeline.my_pipeline.updated_at
depends_on = [module.pipeline]
}
```
Expand All @@ -89,4 +88,3 @@ resource "mezmo_publish_pipeline" "publisher" {
### Required

- `pipeline_id` (String) The id of the pipeline to monitor for publishing. Any changes to its components will trigger a publish. This pipeline must be configured in a child module with an `output`.
- `updated_at` (String) The timestamp of the pipeline's last update. This should always be a reference to the `updated_at` field of the child module's `output` of the pipeline to be pusblished.
1 change: 0 additions & 1 deletion examples/resources/mezmo_publish_pipeline/resource.tf
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,5 @@ module "pipeline" {

resource "mezmo_publish_pipeline" "publisher" {
pipeline_id = module.pipeline.my_pipeline.id
updated_at = module.pipeline.my_pipeline.updated_at
depends_on = [module.pipeline]
}
3 changes: 1 addition & 2 deletions internal/client/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,5 @@ type SharedSource struct {
}

type PublishPipeline struct {
PipelineId string `json:"id"`
UpdatedAt *time.Time `json:"updated_at"`
PipelineId string `json:"id"`
}
18 changes: 0 additions & 18 deletions internal/provider/models/publish_pipeline.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package models

import (
"time"

"github.com/hashicorp/terraform-plugin-framework/resource/schema"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/planmodifier"
"github.com/hashicorp/terraform-plugin-framework/resource/schema/stringplanmodifier"
Expand All @@ -12,7 +10,6 @@ import (

type PublishPipelineResourceModel struct {
PipelineId StringValue `tfsdk:"pipeline_id"`
UpdatedAt StringValue `tfsdk:"updated_at"`
}

func PublishPipelineResourceSchema() schema.Schema {
Expand Down Expand Up @@ -55,34 +52,19 @@ resource "mezmo_pipeline" "my_pipeline" {
stringplanmodifier.RequiresReplace(),
},
},
"updated_at": schema.StringAttribute{
Description: "The timestamp of the pipeline's last update. This should always " +
"be a reference to the `updated_at` field of the child module's `output` of the pipeline " +
"to be pusblished.",
Required: true,
PlanModifiers: []planmodifier.String{
stringplanmodifier.RequiresReplace(),
},
},
},
}
}

// From terraform schema/model to a struct for sending to the API
func PublishPipelineFromModel(plan *PublishPipelineResourceModel) *PublishPipeline {
updatedAt, _ := time.Parse(time.RFC3339Nano, plan.UpdatedAt.ValueString())

publishPipeline := PublishPipeline{
PipelineId: plan.PipelineId.ValueString(),
UpdatedAt: &updatedAt,
}
return &publishPipeline
}

// From an API response to a terraform model
func PublishPipelineToModel(plan *PublishPipelineResourceModel, publishPipeline *PublishPipeline) {
plan.PipelineId = NewStringValue(publishPipeline.PipelineId)
if publishPipeline.UpdatedAt != nil {
plan.UpdatedAt = NewStringValue(publishPipeline.UpdatedAt.Format(time.RFC3339Nano))
}
}
21 changes: 9 additions & 12 deletions internal/provider/publish_pipeline_resource.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,14 +67,7 @@ func (r *PublishPipelineResource) Create(ctx context.Context, req resource.Creat
return
}
publish := PublishPipelineFromModel(&plan)

// The "trick" here is that we don't actually want to change any of the
// values based on the response. The response here will have the most
// recent `updated_at` timestamp, but we need it to stay consistent
// with the state of the pipeline's `updated_at` timestamp. This way,
// we never have the plan values deviate from what's stored in the DB,
// and the parent pipeline can trigger another publish when it refreshes
// and sees that the `updated_at` timestamp has changed on the server.
// Result doesn't matter, in fact it would cause data inconsistencies if we used it.
_, err := r.client.PublishPipeline(publish.PipelineId, ctx)

if err != nil && err.(client.ApiResponseError).Code != "ENOCHANGES" {
Expand All @@ -85,7 +78,10 @@ func (r *PublishPipelineResource) Create(ctx context.Context, req resource.Creat
return
}

// Set the state to match the values from the parent pipeline
// Set the state to satisfy the requirement that the response state be set in this method.
// The value of state is not used because it's removed on every `Read()`, but we need to prevent
// data inconsistecies between the plan and result. There's no need to instantiate
// a model here because it's only `pipeline_id`, so we can just use the `plan` as-is.
diags := resp.State.Set(ctx, plan)
setDiagnosticsHasError(diags, &resp.Diagnostics)
}
Expand All @@ -99,9 +95,10 @@ func (r *PublishPipelineResource) Metadata(_ context.Context, req resource.Metad
}

func (r *PublishPipelineResource) Read(ctx context.Context, req resource.ReadRequest, resp *resource.ReadResponse) {
// NOT having a `Read` for this resource is imperative to how it functions.
// We always want the parent pipeline to control the changing of this resource
// via `RequiresReplace`, so we never want to refresh the schema here.
// This is the magic. We *always* want the publish to fire, so force it to be re-created on every plan
// by removing it from state. Without doing this, the resource might not be selected for a "change" and would
// skip publishing most times.
resp.State.RemoveResource(ctx)
}

func (*PublishPipelineResource) Schema(_ context.Context, _ resource.SchemaRequest, resp *resource.SchemaResponse) {
Expand Down
6 changes: 2 additions & 4 deletions internal/provider/publish_pipeline_resource_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,13 @@ func TestPublishPipelineResource(t *testing.T) {
}
resource "mezmo_publish_pipeline" "my_publish_pipeline" {
pipeline_id = mezmo_pipeline.my_pipeline.id
updated_at = mezmo_pipeline.my_pipeline.updated_at
}
`,
ExpectNonEmptyPlan: true, // We always re-create which causes a non-empty plan.
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("mezmo_pipeline.my_pipeline", "title", "pipeline"),
StateHasExpectedValues("mezmo_publish_pipeline.my_publish_pipeline", map[string]any{
"pipeline_id": "#mezmo_pipeline.my_pipeline.id",
"updated_at": "#mezmo_pipeline.my_pipeline.updated_at",
}),
),
},
Expand All @@ -42,14 +41,13 @@ func TestPublishPipelineResource(t *testing.T) {
}
resource "mezmo_publish_pipeline" "my_publish_pipeline" {
pipeline_id = mezmo_pipeline.my_pipeline.id
updated_at = mezmo_pipeline.my_pipeline.updated_at
}
`,
ExpectNonEmptyPlan: true,
Check: resource.ComposeTestCheckFunc(
resource.TestCheckResourceAttr("mezmo_pipeline.my_pipeline", "title", "Updated Pipeline"),
StateHasExpectedValues("mezmo_publish_pipeline.my_publish_pipeline", map[string]any{
"pipeline_id": "#mezmo_pipeline.my_pipeline.id",
"updated_at": "#mezmo_pipeline.my_pipeline.updated_at",
}),
),
},
Expand Down

0 comments on commit 41957c8

Please sign in to comment.