backup_plan: fix usage for advanced_backup_settings param

mandar242 commented 4 months ago

Summary

The module amazon.aws.backup_plan does not provide example of usage advanced_backup_settings and advanced_backup_settings.backup_options param. Also looking at the argspec for the option, not sure if it will work as expected because it's expecting a dict but I what actually gets passed is a string.

https://github.com/ansible-collections/amazon.aws/blob/main/plugins/modules/backup_plan.py#L151-L156 https://github.com/ansible-collections/amazon.aws/blob/main/plugins/modules/backup_plan.py#L383-L393

Issue Type

Bug Report

Component Name

backup_plan

Ansible Version

$ ansible --version

ansible [core 2.15.5]

Collection Versions

$ ansible-galaxy collection list
amazon.aws 9.0.0-dev0a

AWS SDK versions

$ pip show boto boto3 botocore
Name: boto3
Version: 1.28.15
---
Name: botocore
Version: 1.31.54

Configuration

$ ansible-config dump --only-changed

OS / Environment

No response

Steps to Reproduce

---
- name: Example for amazon.aws.backup_plan
  hosts: localhost
  gather_facts: false
  tasks:
    - name: Create a backup plan
      amazon.aws.backup_plan:
        state: present
        backup_plan_name: my-test-backup-plan1
        advanced_backup_settings:
          - resource_type: EC2
            backup_options: {"WindowsVSS": "enabled"}
        tags:
          TagKey1: TagValue1
          TagKey2: TagValue2
        rules:
          - rule_name: present
            target_backup_vault_name: my-test-vault
            schedule_expression: 'cron(0 5 ? * * *)'
            lifecycle:
              move_to_cold_storage_after_days: 1
              delete_after_days: 91
            recovery_point_tags:
              Key1: Value1
              Key2: Value2
            copy_actions:
              - destination_backup_vault_arn: arn:aws:backup:us-west-2:123456789:backup-vault:my-test-vault
                lifecycle:
                  move_to_cold_storage_after_days: 1
                  delete_after_days: 91
      register: create_result

Create a backup_plan using the option similar to above

Expected Results

The usage of param is advanced_backup_settings verified and fixed if required.

Actual Results

Code of Conduct

[X] I agree to follow the Ansible Code of Conduct

GomathiselviS commented 3 months ago

Hello @mandar242 , what leads you to believe that a string is being passed for backup_options? Could you kindly provide a reference? From my observations, everything appears to be functioning as intended. Here's the value of the create parameters passed to the create_backup_plan CLI.

{'BackupPlan': {'AdvancedBackupSettings': [{'BackupOptions': {'Windowsvss': 'enabled'}, 'ResourceType': 'EC2'}], 'BackupPlanName': 'elasticcopy', 'Rules': [{'CompletionWindowMinutes': 1440, 'EnableContinuousBackup': False, 'RuleName': 'daily', 'ScheduleExpression': 'cron(0 5 ? * * *)', 'StartWindowMinutes': 60, 'TargetBackupVaultName': 'vault1'}]}}

mandar242 commented 3 months ago

Hello @mandar242 , what leads you to believe that a string is being passed for backup_options? Could you kindly provide a reference? From my observations, everything appears to be functioning as intended. Here's the value of the create parameters passed to the create_backup_plan CLI.

{'BackupPlan': {'AdvancedBackupSettings': [{'BackupOptions': {'Windowsvss': 'enabled'}, 'ResourceType': 'EC2'}], 'BackupPlanName': 'elasticcopy', 'Rules': [{'CompletionWindowMinutes': 1440, 'EnableContinuousBackup': False, 'RuleName': 'daily', 'ScheduleExpression': 'cron(0 5 ? * * *)', 'StartWindowMinutes': 60, 'TargetBackupVaultName': 'vault1'}]}}

Hi @GomathiselviS , I could be missing something about the param backup_options being passed as a string. however there are a couple of things that could be improved.

The way backup_options is defined in argspec, is the only module that defines it like choices=[{"WindowsVSS": "enabled"}, {"WindowsVSS": "disabled"}], whereas all other modules make use of suboptions. To ensure consistency, I feel we should modify the way it is written currently and changed to use suboptions instead.
The current definition is causing error with docs rendering https://github.com/ansible-collections/amazon.aws/pull/2106#issuecomment-2124453168
There is no example that showcases usage of advanced_backup_settings and advanced_backup_settings.backup_options

GomathiselviS commented 3 months ago

Hello @mandar242 , what leads you to believe that a string is being passed for backup_options? Could you kindly provide a reference? From my observations, everything appears to be functioning as intended. Here's the value of the create parameters passed to the create_backup_plan CLI. {'BackupPlan': {'AdvancedBackupSettings': [{'BackupOptions': {'Windowsvss': 'enabled'}, 'ResourceType': 'EC2'}], 'BackupPlanName': 'elasticcopy', 'Rules': [{'CompletionWindowMinutes': 1440, 'EnableContinuousBackup': False, 'RuleName': 'daily', 'ScheduleExpression': 'cron(0 5 ? * * *)', 'StartWindowMinutes': 60, 'TargetBackupVaultName': 'vault1'}]}}

Hi @GomathiselviS , I could be missing something about the param backup_options being passed as a string. however there are a couple of things that could be improved.

The way backup_options is defined in argspec, is the only module that defines it like choices=[{"WindowsVSS": "enabled"}, {"WindowsVSS": "disabled"}], whereas all other modules make use of suboptions. To ensure consistency, I feel we should modify the way it is written currently and changed to use suboptions instead.

The current definition is causing error with docs rendering Update return block of Backup modules #2106 (comment)

There is no example that showcases usage of advanced_backup_settings and advanced_backup_settings.backup_options

I will create a PR to update the examples and argspec.

ansible-collections / amazon.aws