-
Notifications
You must be signed in to change notification settings - Fork 204
test: Ensures project withDefaultAlertsSettings
works with import and introduce create_only plan modifier
#3105
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: master
Are you sure you want to change the base?
Conversation
APIx bot: a message has been sent to Docs Slack channel |
…ore in project tests when it is not set to `false` in earlier steps
// we need to reset defaults from what was previously in the state: | ||
// https://discuss.hashicorp.com/t/boolean-optional-default-value-migration-to-framework/55932 | ||
projectPlanNewPtr.WithDefaultAlertsSettings = projectPlan.WithDefaultAlertsSettings | ||
// After import the state&plan will be null so we set to true (default value) |
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.
The user will still get a plan change after import if they have set with_default_alert_settings=false
in their config. But we don't have access to that information in the import
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.
Since this is still a possibility, I'm thinking if we should even implement this fix. This sounds like it should be expected behavior that WithDefaultAlertsSettings will not be set during import and maybe something we can call out in our documentation instead.
I'd find it harder to explain to users why import shows an update if with_default_alert_settings=false
in their config but works when it's true.
if projectPlanNewPtr.WithDefaultAlertsSettings.IsNull() && projectPlan.WithDefaultAlertsSettings.IsNull() { | ||
projectPlanNewPtr.WithDefaultAlertsSettings = types.BoolValue(true) | ||
} else { | ||
// Force value from plan as this is not returned from the API to avoid inconsistent result errors |
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.
it just feels a workaround from an API limitation, have we reached out to the team owning this feature?
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 understand is a create-only parameter that has a side effect of creating alert settings, but I'll check with upstream. Api Docs
@EspenAlbert nit: can we have a why in the summary section to explain the change? also let's hold this merge until we have a conversation with the upstream team (if we did, we should connect the CLOUDPs and understand the why on their side) |
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.
LGTM
…ibute handling to support multiple planmodifiers
…ibutePlanModifier in resource schemas
Will get back to this on Wednesday |
planmodifier.Bool | ||
} | ||
|
||
func CreateOnlyAttributePlanModifier() Modifier { |
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.
Can we add a comment on what this plan modifier does & why it's needed?
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! |
…or_with_default_alert_settings
…support and refactor IsKnown utility function
orgID = os.Getenv("MONGODB_ATLAS_ORG_ID") | ||
projectOwnerID = os.Getenv("MONGODB_ATLAS_PROJECT_OWNER_ID") | ||
projectName = acc.RandomProjectName() | ||
importResourceName = resourceName + "2" |
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.
This test case is a bit different than usual, I tried to add comments, but let me know if anything is unclear.
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.
@EspenAlbert let's hold on this one. I am not too convinced we are doing the right thing in adding more complexity to handle something like this. Let's chat in today's tech sync.
withDefaultAlertsSettings
works with import and introduce create_only plan modifier
Description
Link to any related issue(s): CLOUDP-302586 & #3103
Updated context:
with_default_alert_settings
, before this was only set on create and never returned.During Plan
true
should match what the API returns, TODO: CHECK ME in a migration testterraform plan
, see the error message below.The user will be forced to update their config to match the API value, otherwise they will get inconsistent result value.
During Update
Before this change, Terraform allowed updates to
with_default_alert_settings
, but now we will raise an error:Error message:
During Import
Import works now due to the API change, and the experience will be the same as described in
During Plan
aboveType of change:
Required Checklist:
Further comments