-
Notifications
You must be signed in to change notification settings - Fork 340
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
rds_param_group: WARN on updating DBParameterGroupFamily (engine) #1169
rds_param_group: WARN on updating DBParameterGroupFamily (engine) #1169
Conversation
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
plugins/modules/rds_param_group.py
Outdated
db_parameter_group_family = group['DBParameterGroupFamily'] | ||
|
||
if module.params.get('engine') != db_parameter_group_family: | ||
module.fail_json(msg="The DB parameter group family (engine) can't be changed when updating a DB parameter group.") |
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 the API doesn't originally fail, wouldn't it make more sense to only warn instead of fail? It there isn't any other update to do, it would show up the warning and changed=False. This is only a suggestion.
What do you think @mandar242 @tremble @jillr ?
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.
@alinabuzachis that seems reasonable, I was going back and forth on this but later decided to fail
on it
as there could be case where user wants to update more parameter (modifiable) along with DBParameterGroupFamily
, if we give a warn
instead of fail
, and modify parameters that are possible, it could be easily misunderstood as original reported has mentioned that DBParameterGroupFamily
is also changed.
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's be really clear that this is an immutable option in the docs (I added a small suggestion) and then give the user a warning, that way we don't introduce a breaking change and we're consistent with the API.
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.
@alinabuzachis, as @jillr suggested, changed the failure to warning, to avoid introducing breaking changes.
Now the task shows a warning as below
TASK [rds_param_group : test modifying rds parameter group engine/family] ******
[WARNING]: The DB parameter group family (engine) can't be changed when
updating a DB parameter group.
ok: [testhost] => {"changed": false .....
plugins/modules/rds_param_group.py
Outdated
db_parameter_group_family = group['DBParameterGroupFamily'] | ||
|
||
if module.params.get('engine') != db_parameter_group_family: | ||
module.fail_json(msg="The DB parameter group family (engine) can't be changed when updating a DB parameter group.") |
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's be really clear that this is an immutable option in the docs (I added a small suggestion) and then give the user a warning, that way we don't introduce a breaking change and we're consistent with the API.
SUMMARY
Fixes #1074
Added a check to
WARN
the task while modifying/updating rds_param_group if task tries to changeDB parameter group family
.As AWS Documentation specifies that,
ISSUE TYPE
COMPONENT NAME
rds_param_group
ADDITIONAL INFORMATION
Steps to reproduce