Refactor data retention policy endpoints to allow polymorphic DRP types

[JarrettSpiker]

Description

The data retention policy APIs (a TFE only feature) in the 202401-1 TFE Release have be updated return polymorphic data retention policy types.

Previously the workspace and organization (TFE versions v202311-1 and v202312-1) had a data-retention-policy relationship which would contain a data-retention-policies type. The workspace and org /relationships/data-retention-policy endpoint was used to read and update the drp, accepting and returning the data-retention-policies type.

In TFE version v202401-1, the TFC/E backend no longer returns a data-retention-policies type, instead it uses different types for different drp settings. There is a data-retention-policy-delete-olders for drps which should delete data after N days, and data-retention-policy-dont-deletes for drps which explicitly specify that data should be retained. DRPs can also be null if the workspace/org should inherit a parent DRP.

This causes a failure in go-tfe when trying to read an organization or workspace which contains a DRP from TFE v202401-1, because the returned relationship type value (eg. data-retention-policy-delete-olders) doesnt match what go-tfe expects (data-retention-policies).
To fix this, we need to change the DRP relationships to be polyrelation. Now the relationship on orgs and workspaces, and the DRP read functions, will return a choice type (DataRetentionPolicyChoice) with one populated field containing the DRP.

For updating DRPs, organizations and workspaces have separate functions for setting each type of DRP (SetDataRetentionPolicyDeleteOlder and SetDataRetentionPolicyDontDelete).

In terms of backward compatibility, the "legacy" DataRetentionPolicy type has been retained as one of the options for the polymorphic relationship, and has been marked as deprecated. Similarly the workspace and organization interface SetDataRetentionPolicy function has been maintained, but it will only work with TFE versions v202311-1 and v202312-1...newer TFE versions will need to use the new type specific functions (SetDataRetentionPolicyDeleteOlder and SetDataRetentionPolicyDontDelete).

However, older versions of go-tfe will fail to read DRPs from TFE v202401-1 and newer, when they encounter the data-retention-policy-delete-olders type from the server.

Testing plan

1. For workspaces and organizations in TFE v202401-1 test that
   1. The resource has a relationship to a data-retention-policy, and the type of that relationship matches the type of the drp on the backed
   2. Null DRPS, Delete Older DRPs, and Dont Delete DRPs can be read using the ReadDataRetentionPolicy function
   3. Delete Older DRPs, and Dont Delete DRPs can be created or updated using SetDataRetentionPolicyDeleteOlder and SetDataRetentionPolicyDontDelete respectively. This can update the current drp to have a different value (eg the number of days to delete after), or it can update the type of the DRP (from delete older to dont delete, or vice versa)
   4. DRPs can be deleted with the DeleteDataRetentionPolicy function, which sets the backend to inherit the parent DRP
2. For workspaces and organization in TFE v202311-1 and v202312-1
   1. The choice struct on the DataRetentionPolicy relationship has the legacy DataRetentionPolicy.DataRetentionPolicy field populated
   2. Reading the DRP using ReadDataRetentionPolicy returns a populated legacy DataRetentionPolicy filed on the choice struct
   3. The deprecated SetDataRetentionPolicy can still be used to update the DRP

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

go test -timeout 30s -run ^TestOrganization_DataRetentionPolicy$ github.com/hashicorp/go-tfe

=== RUN   TestOrganization_DataRetentionPolicy
=== RUN   TestOrganization_DataRetentionPolicy/set_and_update_data_retention_policy_to_delete_older
--- PASS: TestOrganization_DataRetentionPolicy/set_and_update_data_retention_policy_to_delete_older (1.23s)
=== RUN   TestOrganization_DataRetentionPolicy/set_data_retention_policy_to_not_delete
--- PASS: TestOrganization_DataRetentionPolicy/set_data_retention_policy_to_not_delete (0.50s)
=== RUN   TestOrganization_DataRetentionPolicy/change_data_retention_policy_type
--- PASS: TestOrganization_DataRetentionPolicy/change_data_retention_policy_type (1.44s)
=== RUN   TestOrganization_DataRetentionPolicy/delete_data_retention_policy
--- PASS: TestOrganization_DataRetentionPolicy/delete_data_retention_policy (0.35s)
--- PASS: TestOrganization_DataRetentionPolicy (5.33s)
PASS
ok      github.com/hashicorp/go-tfe     5.592s


go test -timeout 30s -run ^TestWorkspace_DataRetentionPolicy$ github.com/hashicorp/go-tfe

=== RUN   TestWorkspace_DataRetentionPolicy
=== RUN   TestWorkspace_DataRetentionPolicy/set_and_update_data_retention_policy_to_delete_older
--- PASS: TestWorkspace_DataRetentionPolicy/set_and_update_data_retention_policy_to_delete_older (1.03s)
=== RUN   TestWorkspace_DataRetentionPolicy/set_data_retention_policy_to_not_delete
--- PASS: TestWorkspace_DataRetentionPolicy/set_data_retention_policy_to_not_delete (0.44s)
=== RUN   TestWorkspace_DataRetentionPolicy/change_data_retention_policy_type
--- PASS: TestWorkspace_DataRetentionPolicy/change_data_retention_policy_type (1.24s)
=== RUN   TestWorkspace_DataRetentionPolicy/delete_data_retention_policy
--- PASS: TestWorkspace_DataRetentionPolicy/delete_data_retention_policy (0.29s)
--- PASS: TestWorkspace_DataRetentionPolicy (4.93s)
PASS
ok      github.com/hashicorp/go-tfe
...

JIRA

[JarrettSpiker]

This creates three different functions for creating the three different types of DRP.

An alternative approach would be to have one function which accepts some kind of enum to specify which type of DRP should be created....maybe the DataRetentionPolicySetOptions would contain the type, and then optionally the different attributes for the different types of DRP.

I went for more explicit function signatures, but open to feedback

[JarrettSpiker]

This is potentially something we could improve in the jsonapi fork...it would be useful to specify that the direct result of an API (not just a relationship) is polymorphic, and should populate a choice type.

Im not sure exactly what that would look like...maybe we would also need an UnmarshalPolymorphicPayload function. Or we could add some sort of tag to the choice type (DataRetentionPolicyChoice) to indicate that it should be treated as polymorphic...something like:

type DataRetentionPolicyChoice struct {
       Type string `jsonapi:"primary,polymorphic"`
	DataRetentionPolicy            *DataRetentionPolicy
	DataRetentionPolicyDeleteOlder *DataRetentionPolicyDeleteOlder
	DataRetentionPolicyDontDelete  *DataRetentionPolicyDontDelete
}

[JarrettSpiker]

I am not sure if this should be like this, where we return a nil or empty *DataRetentionPolicyChoice if the org has no DRP....or if we should return a ResourceNotFound error. Open to feedback

[JarrettSpiker]

I have confirmed that this still allows for management of data retention policies on TFE v202312-1...the deprecated DataRetentionPolicy type will still be populated on read, and update works through the deprecated SetDataRetentionPolicy

[brandonc]

The concern is that this is a breaking change to the package API because it was never labeled as beta (and subject to change)

But I also note that this endpoint doesn't appear to be documented at all? Changes to go-tfe presume API changes will follow the stability policy which usually means that we would add an additional method to deal with the new responses, while maintaining the old ones until fully deprecated. Was the stability policy overlooked because it was not documented? Maybe you can tell me more. Nevertheless, the semver of go-tfe reflects the API of the public interfaces, so we have to do something bad. Here are some potential options:

1. We could just rev the version of go-tfe to 2.0. I don't mind this and tbh it could be good since we could align with 'TFC API v2' The downside is that we haven't yet identified other breaking changes that we'd like to include.
2. We could deprecate the existing method but otherwise leave it unchanged, but this has several downsides: it introduces confusing naming, plus it's not only deprecated but literally broken in all but two versions of TFE.

Interested in your thoughts

[JarrettSpiker]

Thanks Brandon, definitely agreed that the backward compatibility breaking is the biggest concern here.

We could deprecate the existing method but otherwise leave it unchanged, but this has several downsides: it introduces confusing naming, plus it's not only deprecated but literally broken in all but two versions of TFE.

We could definitely change this to leave the signature of the ReadDataRetentionPolicy methods unchanged, and add a new ReadDataRetentionPolicyV2 method which returns the choice type....then ReadDataRetentionPolicy would only work when communicating with TFE 202311 and 202312 (we could add a "please use the v2 function" error if it tries to talk to something newer), but ReadDataRetentionPolicyV2 should work with all versions 202311+

That would still leave the issue with the organization and workspace relationships to a data-retention-policy. If we leave the type of that relationship unchanged (DataRetentionPolicy) then I think any time you try to unmarshal an org or workspace which has a data-retention-policy on TFE versions 202401+, it is going to hit an error b/c the type returned by the server wont match the type we declare in the go-tfe struct. We could add a new relationship to orgs and workspaces (maybe DataRetentionPolicyV2), with the goal of only populating one of the two relationships, but Im not sure how we feasible that is.

I can fiddle with it to see what I can get working, but let me know what you think about making those relationships backwards compatible

[JarrettSpiker]

I am in the process of adding API documentation for updating data retention policies through the API. Specifically, the documentation includes information on the different types of data retention policies.

[JarrettSpiker]

API docs for data retention policies now exist for workspaces, organizations, and for the different types of DRPs.

We could deprecate the existing method but otherwise leave it unchanged, but this has several downsides: it introduces confusing naming, plus it's not only deprecated but literally broken in all but two versions of TFE.

@brandonc I have also made a quick alternate version of this which maintains the existing Workspaces and Organizations interface functions, by adding a ReadDataRetentionPolicyV2 rather than changing the return type of ReadDataRetentionPolicy. However, it still requires changing the type of the Organization and Workspace relationships, so backward compatibility isn't totally maintained for anyone integrating with the data retention policy interface.

Im personally not sure which option is better...the one represented in this PR has a cleaner final design, but is a bit worse for backward compatibility, though neither are perfect. Let me know what you think!

[JarrettSpiker]

I found a way to maintain backward compatibility on the Organization and Workspace structs and interfaces.

I have maintained the existing ReadDataRetentionPolicy function on the interface, which still returns the deprecated DataRetentionPolicy type. In order to do this I have also added a ReadDataRetentionPolicyV2 which returns the polymorphic DataRetentionPolicyChoice type. One caveat here is that ReadDataRetentionPolicy will error against TFE 202401-1+, but there is an error message instructing the user on how to fix the issue. If we want to avoid this error, that would also be possible by making multiple API requests to determine the DRP type before attempting to read it.

To maintain the Organization and Workspace structs, I removed the struct tags from the DataRetentionPolicy so that we will not attempt to deserialize into it an encounter the polymorphism error. I have marked that relationship as deprecated and added a new DataRetentionPolicyChoice relationship. When reading the org/workspace, we will manually populate the legacy relationship where applicable. We dont need to worry about the Update case, as updating the DRP via the struct isnt supported anyway.

[brandonc]

Just a few code observations for now. Great work keeping backwards compatibility

[brandonc]

Just pointing out that this is the first time we've revised an interface method with "V2" and it's worth considering other naming since it would be nice to reserve the right to imply version specifiers as they relate to the platform API version.

Other options (just spitballing)

• Create a new interface tfe.DataRetentionPolicies with Read and Set... methods.
• ReadDataRetentionPolicy2 (The lack of a V is a minor difference but could subtly suggest that it's not related to a semantic version of something)
• ReadDataRetentionPolicyChoice (reflects the new return type but is mostly meaningless naming)

[JarrettSpiker]

A potential problem with the new interface (if I am understanding you idea correctly), is that we will also support policies at different levels (admin, project). Admin data retention policies exist, but you cant set "dont delete" policies at that level. So making the AdminGeneralSetting struct implement tfe.DataRetentionPolicies would involve implementing the SetDataRetentionPolicyDontDelete function....which would be not supported.

Maybe not a deal breaker, but a potential reason not to go that route.

Of those choices I would lean ReadDataRetentionPolicyChoice

[JarrettSpiker]

I have updated this to ReadDataRetentionPolicyChoice instead

[brandonc]

💯

[github-actions]

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.