-
Notifications
You must be signed in to change notification settings - Fork 190
feat: Adds support for new description
field in mongodbatlas_resource_policy
resource & data sources
#3214
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
Conversation
mongodbatlas_resource_policy
resource & data sourcesdescription
field in mongodbatlas_resource_policy
resource & data sources
Fixing the examples check |
APIx bot: a message has been sent to Docs Slack channel |
if editAdmin.Description == nil { | ||
editAdmin.Description = conversion.Pointer("") |
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.
why is this needed?
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.
what behavior do we want to have if description is not in the config, setting to empty o keeping the description? (for example in case they change description in the UI, or it's an imported resource with a description but it's not in specified in the config)
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 will be null by default. This particular change is added as it is an optional attribute, so if user wants to "unset" the value/remove it from the config then we send an empty string to the API to do that in the PATCH.
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.
sorry not clear, first sentence and code seem contradictory, and also same question as Oriol, when it's not in the config we want to set it to empty, or leave what it is?
do we need that this code?:
if editAdmin.Description == nil {
editAdmin.Description = conversion.Pointer("")
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.
let makes sure to capture the API and intended terraform behaviour in a code comment
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 have simplified the implementation & added a comment, lmk if it makes the behavior clearer to understand.
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.
when it's not in the config we want to set it to empty, or leave what it is?
@lantoli when it's not in the config we want it to be null, since it is optional. The question is during an update, how do we tell the API to reset it to null in the PATCH request? If we do editAdmin.Description = nil
then it will never be included in the request due to` json:"description,omitempty"``. To address this, we set it to an empty string which the API interprets as 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.
that's fine, thanks
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, with a few comments. I will also ask @erabil-mdb to review since she documented Atlas resource policies.
} | ||
|
||
func configWithPolicyBodies(orgID, policyName string, bodies ...string) string { | ||
func configWithPolicyBodies(orgID, policyName string, description *string, bodies ...string) string { |
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: you can pass description as a string instead of pointer and use it if != ""
@@ -203,7 +213,7 @@ data "mongodbatlas_resource_policy" "test" { | |||
data "mongodbatlas_resource_policies" "test" { | |||
org_id = mongodbatlas_resource_policy.test.org_id | |||
} | |||
`, orgID, policyName, policies) | |||
`, orgID, policyName, descriptionStr, policies) |
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.
we don't need a check for the resource as it's optional but would be useful for the data sources as the field will be computed there
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
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.
Only nit comments
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, lets just clarify the removal aspect
if editAdmin.Description == nil { | ||
editAdmin.Description = conversion.Pointer("") |
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.
let makes sure to capture the API and intended terraform behaviour in a code comment
Description
Adds support for new
description
field inmongodbatlas_resource_policy
resource & data sourcesLink to any related issue(s): CLOUDP-295768
Type of change:
Required Checklist:
Further comments