Skip to content

[DO NOT MERGE] Alternative fix for timezone issue [Issue 2094] #2180

Open
daragh-kearney wants to merge 4 commits intoMyPureCloud:devfrom
daragh-kearney:timezone2
Open

[DO NOT MERGE] Alternative fix for timezone issue [Issue 2094] #2180
daragh-kearney wants to merge 4 commits intoMyPureCloud:devfrom
daragh-kearney:timezone2

Conversation

@daragh-kearney
Copy link
Collaborator

  • Added new attributes callable_time_column_name / contactable_time_column_name and deprecated the old time_column fields.
  • Updated expand/flatten + set hashing and added a small CustomizeDiff to smooth migration.
  • Updated tests/helpers to use and assert the new column_name fields.

Copy link
Contributor

@bbbco bbbco left a comment

Choose a reason for hiding this comment

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

Hi @daragh-kearney , thanks for this

As we spoke about on our call last week, this issue is due to the Genesys Cloud API change to the contact list endpoints. Namely, they are migrating off of using the callableTimeColumn on the phoneColumns and contactableTimeColumn on the emailColumns fields in favor of the callableTimeColumnName and contactableTimeColumnName fields.

While we were on the call, we determined that the API currently still accepts the callableTimeColumn and contactableTimeColumn fields on creation/updates to the API, but when reading this API, these fields are only showing up on the callableTimeColumnName and contactableTimeColumnName fields. However, the purecloud Golang SDK hasn't updated with these fields yet, so we have to implement some workarounds.

I think we need to determine what the plan for addressing this interesting state. Please take a look at my comments and maybe work with @HemanthDogiparthi12 or someone on the CX as Code team to determine the best path forward. It will be important that this is handled in a correct manner, or we risk breaking the ability to sync these resources in the various BCP implementations.

return retry.NonRetryableError(util.BuildWithRetriesApiDiagnosticError(ResourceType, fmt.Sprintf("failed to read Outbound Contact List %s | error: %s", d.Id(), getErr), resp))
}

// The SDK model for phone/email columns does not include the API-returned `*TimeColumnName` fields.
Copy link
Contributor

Choose a reason for hiding this comment

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

You should probably include in this comment a link or JIRA to the corresponding fix for pulling this field into the SDK with instructions to remove this code after the SDK has been upgraded with support for the new field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now added a TODO with a link to the Go SDK repo to remove this once the SDK models include TimeColumnName fields.

return d.Get("automatic_time_zone_mapping").(bool)
},
},
`callable_time_column_name`: {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, I don't know if its absolutely necessary to add a new field to the schema. Basically, there are two approaches you can take:

  1. Add a new field to the schema that matches the name of the new API field, with a deprecation message (what you have implemented in this PR) plus adding a migration path (see the routing_queue resource for how a migration was handled in the past)
  • Pros: Keeps the provider fields 1:1 aligned with the API fields
  • Cons: Requires the user to make changes/modifications to their Terraform code / output via a code migration.
  1. Retain the callable_time_column field. Update the flatten/build functions to read/write to the callable_time_column_name field for this schema field.
  • Pros: Seamless experience from the user's perspective. No migrations or updates to their code necessary.
  • Cons: Deviates from the API so that the field names don't match up 1:1 (callable_time_column does not match callableTimeColumnName). A comment on the description of the field could clarify this aspect for the user.

You should check in with your team leads on which approach is desired.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

After discussing with Hemanth we determined that Approach 1 was desired: I've added the new _time_column_name fields, deprecated the old _time_column fields, and provided a migration path via StateUpgraders using the routing_queue resource as reference.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes this wont break anything for the end user immediately, (thanks to the deprecation) and we can remove this in the later releases. api fields 1:1 aligning with the terraform schema will make sure of consistency in the long run.

Optional: true,
Type: schema.TypeString,
},
`contactable_time_column_name`: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above comment for this field.

Optional: true,
ForceNew: true,
Type: schema.TypeSet,
Set: hashOutboundContactListPhoneColumn,
Copy link
Contributor

Choose a reason for hiding this comment

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

If you hash these values, how is the provider supposed to read the values back?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed set behavior so hashing is only for stable TypeSet identity, and ensured flatten/build keep state consistent:

  • Flatten now uses the same hash function as the schema Set: function.
  • Flatten writes both legacy and new time_column fields so config/state don’t drift and match correctly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Great work identifying that this would be impactful to the contact list template too! Same as we decide to do with contact list should also be done here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now applied the same approach to the contact list template as well.

)

func normalizeOutboundContactListTimeColumnFields(_ context.Context, diff *schema.ResourceDiff, _ interface{}) error {
// Backfill the new *_column_name fields from the legacy *_column fields at plan time.
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't exactly backfilling as you expect. All it is doing is manipulating the diff output so Terraform doesn't see the differences between the two fields as a change. The state is not getting updated, which is what would happen in an actual backfill. However, that should happen within the confines of a schema migration. See my comment further down to determine if that is the approach you want to take.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've now implemented a schema/state migration: bumped SchemaVersion and added StateUpgraders so existing state is actually upgraded.

if callableTimeColumnName, ok := contactPhoneNumberColumnMap["callable_time_column_name"].(string); ok && callableTimeColumnName != "" {
sdkContactPhoneNumberColumn.CallableTimeColumn = &callableTimeColumnName
} else if callableTimeColumn := contactPhoneNumberColumnMap["callable_time_column"].(string); callableTimeColumn != "" {
sdkContactPhoneNumberColumn.CallableTimeColumn = &callableTimeColumn
Copy link
Contributor

Choose a reason for hiding this comment

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

So I think the API is at a point right now where it allows setting the CallableTimeColumn but only getting on the CallableTimeColumnName.

This is fine logic if you are using Option 1. from above, but not needed if you are using Option 2. until the API completely decommissions the CallableTimeColumn field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants