rds_instance_pram_group_info: add new module rds_instance_pram_group_info #2372
Conversation
Docs Build 📝Thank you for contribution!✨ This PR has been merged and your docs changes will be incorporated when they are next published. |
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 5m 59s |
|
Can you add tests to validate this module? |
@GomathiselviS just wanted to know thoughts on whether the approach taken for module looks good or not before moving ahead with adding tests. Specifically the way module will work. |
| from ansible_collections.amazon.aws.plugins.module_utils.retries import AWSRetry | ||
|
|
||
|
|
||
| def get_db_instance_param_group_name(connection: Any, module: AnsibleAWSModule) -> str: |
There was a problem hiding this comment.
It appears that this function is already addressed by rds_instance_info.py. Do we really need it here? While it does offer additional functionality, I believe it may be redundant in order to align with the rds_cluster_param_group_info module.
There was a problem hiding this comment.
It is redundant however I think it would be good to have that function in the code as it allows below task
First code finds the name of parameter group attached to the rds instance, and uses it to find that param group's info
- name: Get specific DB instance's parameter group info
amazon.aws.rds_instance_param_group_info:
db_instance_identifier: database-1
above can be surely achieved with combination of existing rds_instance_info.py and this new module but reduces user experience in my opinion.
- name: Get specific DB instance's parameter group info
amazon.aws.rds_instance_info:
db_instance_identifier: database-1
register: instance_info
- name: Get specific DB parameter group's info
amazon.aws.rds_instance_param_group_info:
db_parameter_group_name: "{{ instance_info.instances[0].db_parameter_groups.db_parameter_group_name }}"
I'm open with going with either approach. @GomathiselviS
There was a problem hiding this comment.
I still believe this is redundant. If we aim to add extra functionalities, there are many possibilities. However, the core functionality of a module should align with its intended purpose and maintain consistency with other modules in the collection. Please feel free to bring it up in the team call, and if others have a different perspective, you can go ahead and add this functionality.
I am not sure about the first example here. I have shared my feedback in the review comment for the corresponding function in the module file. The rest looks good. |
mandar242
changed the title
mandar242
force-pushed
the
rds_instance_param_group_info
branch
from
182fca8 to
6f0a471
Compare
I don’t think that you need tu supply the db_instance_identifier for this module. If you want information about an RDS instance you use the rds_instance_info module. The rds_instance_param_group_info should behave similarly to the rds_cluster_param_group_info module. |
|
@GomathiselviS @alinabuzachis remove the redundant functionality. |
changelogs/fragments/2372-rds_instance_pram_group_info-add-new-module.yml
Outdated
Show resolved
Hide resolved
mandar242
force-pushed
the
rds_instance_param_group_info
branch
from
2cc9a58 to
331ff54
Compare
|
CI failing on unrelated, good to merge this PR? @GomathiselviS |
Once #2384 is merged, this PR can be merged by adding "mergeit" label. |
mandar242
force-pushed
the
rds_instance_param_group_info
branch
from
317166f to
90562d1
Compare
softwarefactory-project-zuul
bot
merged commit b48adec
into
ansible-collections:main
Backport to stable-9: 💚 backport PR created✅ Backport PR branch: Backported as #2387 🤖 @patchback |
…info (#2372) SUMMARY Adding a new module for retrieving parameter group info. Can get info for a specific RDS parameter group the parameter group associated with a specified RDS instance all parameter groups available in the current region. Resolves #2313 ISSUE TYPE New Module Pull Request COMPONENT NAME rds_instance_pram_group_info ADDITIONAL INFORMATION Created basic module, waiting on confirmation to move forward with adding tests, etc. Reviewed-by: GomathiselviS <[email protected]> Reviewed-by: Mandar Kulkarni <[email protected]> Reviewed-by: Alina Buzachis (cherry picked from commit b48adec)
…info (#2372) (#2387) This is a backport of PR #2372 as merged into main (b48adec). SUMMARY Adding a new module for retrieving parameter group info. Can get info for a specific RDS parameter group the parameter group associated with a specified RDS instance all parameter groups available in the current region. Resolves #2313 ISSUE TYPE New Module Pull Request COMPONENT NAME rds_instance_pram_group_info ADDITIONAL INFORMATION Created basic module, waiting on confirmation to move forward with adding tests, etc. Reviewed-by: Alina Buzachis
SUMMARY
Adding a new module for retrieving parameter group info.
Can get info for
Resolves #2313
ISSUE TYPE
COMPONENT NAME
rds_instance_pram_group_info
ADDITIONAL INFORMATION
Created basic module, waiting on confirmation to move forward with adding tests, etc.