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

Jump to bottom

refactor(product_enablement): make Read() logic consistent with other resource types #773

Merged
merged 1 commit into from
Oct 18, 2023
Merged

refactor(product_enablement): make Read() logic consistent with other resource types #773

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
66 changes: 24 additions & 42 deletions fastly/block_fastly_service_product_enablement.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,52 +174,29 @@ func (h *ProductEnablementServiceAttributeHandler) Create(_ context.Context, d *
func (h *ProductEnablementServiceAttributeHandler) Read(_ context.Context, d *schema.ResourceData, _ map[string]any, _ int, conn *gofastly.Client) error {
localState := d.Get(h.Key()).(*schema.Set).List()

// IMPORTANT: Avoid a diff unless state actually contains product_enablement.
//
// For all other nested blocks we go to the API to get the latest data if the
// user has already configured the block in their config (hence it exists in
// their state file) OR if the user is doing a `terraform import` OR if the
// user has changed the active service version outside of Terraform and the
// provider now needs to re-sync so it's tracking the correct version.
//
// But that approach means that users who don't configure a product_enablement
// block but fall into the other two categories (i.e. importing or service
// version drift) will unexpectedly see a `product_enablement` block in their
// next plan/diff output and it will be suggesting that the block be deleted.
//
// This causes confusion for users and they're not sure how to resolve the
// diff and sometimes will ADD the block into their config and try to set
// different values, which then causes other errors as they may not be
// entitled to actually use the Product Enablement API.
//
// To avoid this confusion, for product_enablement only, we only go to the API
// if the user has either configured the product_enablement block in their
// config or if they've configured it AND they're either importing or if there
// is a force refresh due to the service version drifting.
//
// The reason we have to treat the Product Enablement (PE) API differently is
// because its access is restricted to entitled users and so unlike other API
// endpoints (e.g. backends or domains etc), including calls to the PE API can
// cause unexpected diffs and runtime errors that stop a plan from being
// applied successfully.
existsInState := len(localState) > 0
if existsInState || (existsInState && (d.Get("imported").(bool) || d.Get("force_refresh").(bool))) {
if len(localState) > 0 || d.Get("imported").(bool) || d.Get("force_refresh").(bool) {
log.Printf("[DEBUG] Refreshing Product Enablement Configuration for (%s)", d.Id())

// The API returns a 400 if a product is not enabled.
// The API client returns an error if a non-2xx is returned from the API.

// NOTE: We must set name to a unique value for a plan diff to be calculated.
result := map[string]any{}

// The `name` attribute in this resource is used by default as a key for calculating diffs.
// This is handled as part of the internal abstraction logic.
//
// This value can be static because (like with the 'package' block) there can
// only ever be one 'product_enablement' block per service resource.
// This is ensured via the schema where we set MinItems/MaxItems to 1.
// See the call ToServiceAttributeDefinition() inside NewServiceProductEnablement()
// See also the diffing logic:
// - https://github.com/fastly/terraform-provider-fastly/blob/4b9506fba1fd17e2bf760f447cbd8c394bb1e153/fastly/service_crud_attribute_definition.go#L94
// - https://github.com/fastly/terraform-provider-fastly/blob/4b9506fba1fd17e2bf760f447cbd8c394bb1e153/fastly/diff.go#L108-L117
//
// Unlike the 'package' block we use a structure copied from 'backend'.
// This is done so we can benefit from the 'modified' map data passed to the
// 'update' CRUD method.
result := map[string]any{
"name": localState[0].(map[string]any)["name"].(string),
// Because the name can be set by a user, we first check if the resource
// exists in their state, and if so we'll use the value assigned there. If
// they've not explicitly defined a name in their config, then the default
// value will be returned.
if len(localState) > 0 {
name := localState[0].(map[string]any)["name"].(string)
result["name"] = name
}

if h.GetServiceMetadata().serviceType == ServiceTypeCompute {
Expand Down Expand Up @@ -270,9 +247,14 @@ func (h *ProductEnablementServiceAttributeHandler) Read(_ context.Context, d *sc

results := []map[string]any{result}

if err := d.Set(h.Key(), results); err != nil {
log.Printf("[WARN] Error setting Product Enablement for (%s): %s", d.Id(), err)
return err
// IMPORTANT: Avoid runtime panic "set item just set doesn't exist".
// TF will panic when trying to append an empty map to a TypeSet.
// i.e. a typed nil.
if len(results[0]) > 0 {
if err := d.Set(h.Key(), results); err != nil {
log.Printf("[WARN] Error setting Product Enablement for (%s): %s", d.Id(), err)
return err
}
}
}

Expand Down