chore: Adds support for plan modifier on replication_specs changed #3083

EspenAlbert · 2025-02-18T11:16:02Z

Description

Adds support for plan modifier on replication_specs changed

Link to any related issue(s): CLOUDP-300282

Full Runs in TPF:

Type of change:

Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR. A migration guide must be created or updated if the new feature will go in a major version.
Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR. A migration guide must be created or updated.
This change requires a documentation update
Documentation fix/enhancement

Required Checklist:

I have signed the MongoDB CLA
I have read the contributing guides
I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
I have added any necessary documentation (if appropriate)
I have run make fmt and formatted my code
If changes include deprecations or removals I have added appropriate changelog entries.
If changes include removal or addition of 3rd party GitHub actions, I updated our internal document. Reach out to the APIx Integration slack channel to get access to the internal document.

Further comments

…ould be kept unknown

…es that are unknown

…tions

…keepUnknown logic for changed replication specs

…management and error diagnostics

…pUnknowns logic

… unused clusterUseISS function

…plication specs

…eKeepUnknowns

… sending `UseStateForUnknown` values

…urce and destination TFModels

…modifications

…nfig import

…e changes and clean up unused functions

…r better list change detection

…ecs changes

…mproved change detection

…ify related logic by using AttributeChanges

EspenAlbert · 2025-02-20T14:22:34Z

internal/service/advancedclustertpf/model_to_ClusterDescription20240805.go

@@ -95,8 +95,8 @@ func newReplicationSpec20240805(ctx context.Context, input types.List, diags *di
 	for i := range elements {
 		item := &elements[i]
 		resp[i] = admin.ReplicationSpec20240805{
-			Id:            conversion.NilForUnknown(item.ExternalId, item.ExternalId.ValueStringPointer()),
-			ZoneId:        conversion.NilForUnknown(item.ZoneId, item.ZoneId.ValueStringPointer()),
+			Id:            conversion.NilForUnknownOrEmptyString(item.ExternalId),


Doesn't make sense to send an empty string for the id

internal/service/advancedclustertpf/plan_modifier.go

EspenAlbert · 2025-02-20T15:20:19Z

internal/common/schemafunc/find_changes.go

+
+// FindAttributeChanges: Iterates through TFModel of state+plan and returns AttributeChanges for querying changed attributes
+// The implementation is similar to KeepUnknown, no support for types.Set or types.Tuple yet
+func FindAttributeChanges(ctx context.Context, src, dest any) AttributeChanges {


Why another reflect function?

Use the same names as TF and therefore simplify call to CopyUnknowns.

Could re-use the major parts of KeepUnknown

Could have tried to modify PatchPayload to send back attributes changed, but it wasn't a good fit. PatchPayload does 2 things:

(1) Checking for changes on an attribute and for changed attributes

(2) Copies values from state

(3) Require Structs with json annotations to work

Had an attempt at using the path package, but I was unsuccessful.

internal/common/schemafunc/find_changes.go

internal/common/schemafunc/plan_modifier_state_for_unknown.go

…ma functions

…upgrade (goes from "" --> x.x)

…nknowns function

EspenAlbert added 10 commits February 14, 2025 15:20

feat: Index copying of replication specs

14fd489

feat: Index copying of region_configs

ae34781

naive implementation, always CopyUnknowns but has errors

d00dc0d

feat: Support detecting ISS to set empty Id

beabadf

test: Add failing test case for an unknown object with fields that sh…

e10411f

…ould be kept unknown

feat: Enhance CopyUnknowns to respect keepUnknown for object attribut…

53cf721

…es that are unknown

feat: Update CopyUnknowns to respect keepUnknown for region configura…

40eadf5

…tions

feat: Enhance handling of unknowns in replication specs with dynamic …

67dda7f

…keepUnknown logic for changed replication specs

chore: update mock data

2b574d2

refactor: move clusterUseISS to plan_modifier

fcabadc

EspenAlbert mentioned this pull request Feb 18, 2025

chore: Adds support for plan modifier on replication_specs changed (safer) #3084

Closed

13 tasks

EspenAlbert added 19 commits February 18, 2025 15:29

fix: patch_payload forceUpdateAttr must include the attribute

9d4df88

feat: Enhance sharding configuration handling with improved unknowns …

0ca06fa

…management and error diagnostics

feat: Improve unknowns handling in replication specs with dynamic kee…

1032c12

…pUnknowns logic

chore: safeguard conversion to PATCH by never using "" in Id or ZoneId

cee755c

feat: Refactor unknowns handling in sharding configuration and remove…

ae25e87

… unused clusterUseISS function

feat: Add zone name matching logic to enhance unknowns handling in re…

c54ab50

…plication specs

chore: update MacT tests

f912670

refactor: write doc and use the same concepts everywhere and determin…

39fb89e

…eKeepUnknowns

refactor: update Update method to use config instead of plan to avoid…

b90224c

… sending `UseStateForUnknown` values

feat: Implement FindChanges function to detect differences between so…

bd89539

…urce and destination TFModels

feat: Add AttributeChanges struct and methods for tracking attribute …

b5c0f95

…modifications

refactor: Rename config variable to configModel to avoid shadowing co…

ab10624

…nfig import

refactor: Enhance determineKeepUnknowns logic to incorporate attribut…

4bc76fc

…e changes and clean up unused functions

refactor: Use AttributeChanges instead of Patchpayload

605d17b

refactor: Use vars to simplify the logic of which fields to keep unknown

8e22220

test: Add unit tests for KeepUnknown method in AttributeChanges

34e6e7e

refactor: Simplify leafChanges logic and add ListLenChanges method fo…

06b1b1a

…r better list change detection

refactor: External_id should not be set if the size of replication sp…

641cf2c

…ecs changes

refactor: Add ListIndexChanged and NestedListLenChanges methods for i…

6469767

…mproved change detection

EspenAlbert added 3 commits February 20, 2025 14:09

refactor: Enhance handling of unknowns in replication specs and simpl…

4910bca

…ify related logic by using AttributeChanges

chore: update mocked data

47619d9

chore: revert stylistic changes

d36abc7

EspenAlbert commented Feb 20, 2025

View reviewed changes

EspenAlbert added 2 commits February 20, 2025 14:24

doc: Add some comments to describe changes

1185cf2

refactor: Simplify patch options for replication specs in Update method

558fcbc

EspenAlbert commented Feb 20, 2025

View reviewed changes

internal/service/advancedclustertpf/plan_modifier.go Outdated Show resolved Hide resolved

EspenAlbert commented Feb 20, 2025

View reviewed changes

EspenAlbert added 7 commits February 20, 2025 15:22

chore: minor comment [ci skip]

edc4d78

refactor: move fields with no dependencies to the top

caa89e6

feat: add minimize level configuration for replication specs

a356e9f

fix minimize level logic

2aa280d

fix: change default minimize level to always for bug investigation

dff577c

Merge branch 'master' into CLOUDP-300282_plan_minimize_replication_specs

9a6d9c8

merge plan modifier changes cleanup

75c1b7b