Add backup_plan and backup_plan_info modules #1446
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 failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 35s |
There was a problem hiding this comment.
Thanks for this. I've taken a quick first look, and most of the things that are standing out to me are minor rather than something significant.
You'll see there are multiple layers of linter and tests complaining about things.
The "Sanity" tests
ERROR: Found 13 validate-modules issue(s) which need to be resolved:
ERROR: plugins/modules/backup_plan.py:0:0: import-error: Exception attempting to import module for argument_spec introspection, 'No module named 'ansible.module_utils.resource_tags''
ERROR: plugins/modules/backup_plan.py:0:0: parameter-list-no-elements: DOCUMENTATION.options.advanced_backup_settings: Argument defines type as list but elements is not defined for dictionary value @ data['options']['advanced_backup_settings']. Got {'description': ['Specifies a list of BackupOptions for each resource type. These settings are only available for Windows Volume Shadow Copy Service (VSS) backup jobs.'], 'required': False, 'type': 'list'}
ERROR: plugins/modules/backup_plan.py:0:0: parameter-list-no-elements: DOCUMENTATION.options.rules: Argument defines type as list but elements is not defined for dictionary value @ data['options']['rules']. Got {'description': ['An array of BackupRule objects, each of which specifies a scheduled task that is used to back up a selection of resources.'], 'required': False, 'type': 'list'}
ERROR: plugins/modules/backup_plan.py:5:0: import-before-documentation: Import found before documentation variables. All imports must appear below DOCUMENTATION/EXAMPLES/RETURN.
ERROR: plugins/modules/backup_selection.py:0:0: import-error: Exception attempting to import module for argument_spec introspection, 'No module named 'ansible.module_utils.resource_tags''
ERROR: plugins/modules/backup_selection.py:0:0: parameter-list-no-elements: DOCUMENTATION.options.list_of_tags: Argument defines type as list but elements is not defined for dictionary value @ data['options']['list_of_tags']. Got {'description': ['A list of conditions that you define to assign resources to your backup plans using tags. Condition operators are case sensitive.'], 'required': False, 'type': 'list'}
ERROR: plugins/modules/backup_selection.py:0:0: parameter-list-no-elements: DOCUMENTATION.options.not_resources: Argument defines type as list but elements is not defined for dictionary value @ data['options']['not_resources']. Got {'description': ['A list of Amazon Resource Names (ARNs) to exclude from a backup plan. The maximum number of ARNs is 500 without wildcards, or 30 ARNs with wildcards. If you need to exclude many resources from a backup plan, consider a different resource selection strategy, such as assigning only one or a few resource types or refining your resource selection using tags.'], 'required': False, 'type': 'list'}
ERROR: plugins/modules/backup_selection.py:0:0: parameter-list-no-elements: DOCUMENTATION.options.resources: Argument defines type as list but elements is not defined for dictionary value @ data['options']['resources']. Got {'description': ['A list of Amazon Resource Names (ARNs) to assign to a backup plan. The maximum number of ARNs is 500 without wildcards, or 30 ARNs with wildcards. If you need to assign many resources to a backup plan, consider a different resource selection strategy, such as assigning all resources of a resource type or refining your resource selection using tags.'], 'required': False, 'type': 'list'}
ERROR: plugins/modules/backup_selection.py:0:0: return-syntax-error: RETURN.backup_selection.contains.creation_date.sample: not a valid value for dictionary value @ data['backup_selection']['contains']['creation_date']['sample']. Got datetime.datetime(2023, 1, 24, 10, 8, 3, 193000, tzinfo=datetime.timezone(datetime.timedelta(seconds=3600)))
ERROR: plugins/modules/backup_selection.py:5:0: import-before-documentation: Import found before documentation variables. All imports must appear below DOCUMENTATION/EXAMPLES/RETURN.
ERROR: plugins/modules/backup_vault.py:0:0: import-error: Exception attempting to import module for argument_spec introspection, 'No module named 'ansible.module_utils.resource_tags''
ERROR: plugins/modules/backup_vault.py:0:0: return-syntax-error: RETURN.backup_vault.contains.tags.contains: required key not provided @ data['backup_vault']['contains']['tags']['contains']. Got None
ERROR: plugins/modules/backup_vault.py:6:0: import-before-documentation: Import found before documentation variables. All imports must appear below DOCUMENTATION/EXAMPLES/RETURN.
The 'black' linter:
https://github.com/ansible-collections/amazon.aws/actions/runs/4541478098/jobs/8003940319?pr=1446
I've also picked out a few other things.
One minor additional thing I'd usually tweak: It's much easier to follow the code if you break it out into functions: one big "main" with lots of nested statements can be kinda hard to follow.
If you're willing to make these changes that'd be great. You can find some more documentation about writing tests in our docs https://github.com/ansible-collections/amazon.aws/blob/main/docs/docsite/rst/dev_guidelines.rst#integration-tests-for-aws-modules
|
Thanks for reviewing it so quickly, I fixed most of the things. Breaking to functions will need to take a deeper breath, maybe it would make sense to do after test cases are there, so that we can see eventual regressions immediately. I still see a conflict though with #1436 How should we approach this problem? |
That's fine. You've seen the other PRs, does the way we write our integration tests make sense to you? (they're essentially just roles in tests/integration/targets)
I'd probably drop the backup_vault module from this PR, since @GomathiselviS is paid to work on this she's probably likely to be ready with her PR before we could get these merged. |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 50s |
|
One additional comment - Thanks for pushing these, even if you don't have the time to get things finished off, it should be possible to build upon your work and get these modules finished off. Personally I would hope that we get to see some reasonable basic support for AWS Backup in release 6.0.0 (and Ansible 8) |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 04s |
tremble
changed the title
ansibullbot
added
WIP
| returned: always | ||
| type: str | ||
| sample: elastic | ||
| backup_plan_id: |
There was a problem hiding this comment.
I donot see backup_plan_name and backup_plan_id as part of the response structure in the documentation. Are you adding these keys to the return dict?
There was a problem hiding this comment.
many thanks, very good catch, it really returned a null when creating a plan, fixed with 99b2dab
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 04s |
|
Hi @krisek Are you planning to add backup_plan_info and backup_selection_info modules? If not I can start with those modules. |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 3m 36s |
Thanks a lot, I started to work on backup_plan_info based on yours... got stuck a bit with tag retrieval. But it's progressing. |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 00s |
|
@krisek Thanks for all your work on these modules. If you have work that hasn't been pushed yet, feel free to add it. We'll likely try to pick this up and continue on what you've already done, as we'd like to get these into the 6.0.0 release coming up in a couple weeks. |
|
there is one info module missing and tests cases of course, I'll take care about the info module in the next two days |
@krisek Thank your for this contribution. If you need any help, just let us know here or on IRC Libera.Chat #ansible-aws. |
|
@krisek I just joined the team at Ansible and can start work on the tests for these modules. Let me know ASAP if you've already made progress on those though as I don't want to duplicate your work. |
|
just starting to create the missing info module, sorry this week was more turbulent than I expected |
|
Build failed. ✔️ ansible-galaxy-importer SUCCESS in 4m 26s |
plugins/modules/backup_plan_info.py
Outdated
| default: [] | ||
| description: | ||
| - Specifies a list of plan names. | ||
| - If an empty list is specified, information for the backup plans in the current region is returned. |
There was a problem hiding this comment.
I guess we should add note like "Listing all the AWS backup plans is not supported at the moment. When I(backup_plan_names) is not set, an empty list is returned." We can then add this functionality next.
There was a problem hiding this comment.
@tremble What do you suggest?
There was a problem hiding this comment.
Keep it simple. Make it required for now, we can make it optional later as a "new feature" (non-breaking IMO).
There was a problem hiding this comment.
Right, no breaking changes! Makes sense.
There was a problem hiding this comment.
My bad! I guess that functionality works https://github.com/ansible-collections/amazon.aws/pull/1446/files#diff-d2301559aaf1b18099212d421d58fc78b99063e3f730e0c23c1cc79cff553402R114-R121 (missed this part), @hakbailey can you please confirm?
There was a problem hiding this comment.
So, I confirm that it's not working. Just apply @tremble's suggestion:
TASK [Get detailed information about all the AWS Backup plan] *******************************************************************************
task path: /Users/alinabuzachis/dev/backup_modules_demo/automate_backup.yml:63
<127.0.0.1> ESTABLISH LOCAL CONNECTION FOR USER: alinabuzachis
<127.0.0.1> EXEC /bin/sh -c 'echo ~alinabuzachis && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /Users/alinabuzachis/.ansible/tmp `"&& mkdir "` echo /Users/alinabuzachis/.ansible/tmp/ansible-tmp-1683645143.653943-8471-254738348882311 `" && echo ansible-tmp-1683645143.653943-8471-254738348882311="` echo /Users/alinabuzachis/.ansible/tmp/ansible-tmp-1683645143.653943-8471-254738348882311 `" ) && sleep 0'
Using module file /Users/alinabuzachis/dev/collections/ansible_collections/amazon/aws/plugins/modules/backup_plan_info.py
<127.0.0.1> PUT /Users/alinabuzachis/.ansible/tmp/ansible-local-83104tp3qnho/tmpak9_s3dk TO /Users/alinabuzachis/.ansible/tmp/ansible-tmp-1683645143.653943-8471-254738348882311/AnsiballZ_backup_plan_info.py
<127.0.0.1> EXEC /bin/sh -c 'chmod u+x /Users/alinabuzachis/.ansible/tmp/ansible-tmp-1683645143.653943-8471-254738348882311/ /Users/alinabuzachis/.ansible/tmp/ansible-tmp-1683645143.653943-8471-254738348882311/AnsiballZ_backup_plan_info.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c '/Users/alinabuzachis/anaconda3/envs/py310/bin/python /Users/alinabuzachis/.ansible/tmp/ansible-tmp-1683645143.653943-8471-254738348882311/AnsiballZ_backup_plan_info.py && sleep 0'
<127.0.0.1> EXEC /bin/sh -c 'rm -f -r /Users/alinabuzachis/.ansible/tmp/ansible-tmp-1683645143.653943-8471-254738348882311/ > /dev/null 2>&1 && sleep 0'
ok: [localhost] => {
"backup_plans": [],
"changed": false,
"invocation": {
"module_args": {
"access_key": null,
"aws_ca_bundle": null,
"aws_config": null,
"backup_plan_names": [],
"debug_botocore_endpoint_logs": false,
"endpoint_url": null,
"profile": null,
"region": null,
"secret_key": null,
"session_token": null,
"validate_certs": true
}
}
}
…ion's requirements Signed-off-by: Alina Buzachis <[email protected]>
Signed-off-by: Alina Buzachis <[email protected]>
- Refactor backup_plan module - Complete backup_plan integration tests - Update backup_plan_info module to pass sanity tests
alinabuzachis
force-pushed
the
module_backup_support
branch
from
178275c to
6244a76
Compare
Signed-off-by: Alina Buzachis <[email protected]>
alinabuzachis
force-pushed
the
module_backup_support
branch
from
6244a76 to
f4c8bfc
Compare
|
Build succeeded. ✔️ ansible-galaxy-importer SUCCESS in 3m 41s |
|
regate |
|
Build succeeded (gate pipeline). ✔️ ansible-galaxy-importer SUCCESS in 5m 10s |
SUMMARY
These three modules add capability to manage all configurations related to backups on AWS.
ISSUE TYPE
COMPONENT NAME
backup_vault
backup_plan
backup_selection
ADDITIONAL INFORMATION
We've been using these three modules for 6 months now in our AWS estate, I thought it might be useful for others.
I know that this is not the repo for public contribution, but as there is already work being done on this area here, I think it still make sense to have a look on the code, I tried to be as comprehensive as possible.
No worries if it doesn't make into the main branch, feel free to cherry pick parts of it.