-
Notifications
You must be signed in to change notification settings - Fork 189
refactor: Uses new plan modify logic common functions in advanced_cluster TPF #3218
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.
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
base: CLOUDP-307851_common_plan_modifier_logic
Are you sure you want to change the base?
refactor: Uses new plan modify logic common functions in advanced_cluster TPF #3218
Conversation
…on' expected "8.0", got "8.1"
} | ||
electablePath := req.Path.ParentPath().AtName("electable_specs") | ||
electable := customplanmodifier.ReadPlanStructValue[TFSpecsModel](ctx, req.Differ, electablePath) | ||
if electable == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as in previous PR, are we doing the same logic as before this change? In particular, take electableSpecs from state if not in config and node > 0, and keep disk_size unkown if defined in root? (i.e. same logic as in analyticsAndElectableSpecsReplaceUnknown to get electable_specs)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some more tests for this and improved the logic. Can you have a look again and double-check the logic matches?
ConfigStateChecks: []statecheck.StateCheck{ | ||
statecheck.ExpectKnownValue(resourceName, tfjsonpath.New("mongo_db_major_version"), knownvalue.StringRegexp(regexp.MustCompile(`8\.\d+`))), | ||
statecheck.ExpectKnownValue(dataSourceName, tfjsonpath.New("mongo_db_major_version"), knownvalue.StringRegexp(regexp.MustCompile(`8\.\d+`))), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: these changes are not necessary once you rebase with master including #3211
…08782_copy_unknowns_and_block_removal
for attrName, replacer := range attributePlanModifiers { | ||
unknownReplacements.AddReplacement(attrName, replacer) | ||
} | ||
unknownReplacements.AddKeepUnknownAlways("connection_strings", "state_name", "mongo_db_version") // Volatile attributes, should not be copied from state) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the attribute list out of the method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is necessary
…08782_copy_unknowns_and_block_removal
This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
Description
replication_specs.*
if there are changes insidereplication_specs
Link to any related issue(s): CLOUDP-308782
Type of change:
Required Checklist:
Further comments