community.aws icon indicating copy to clipboard operation
community.aws copied to clipboard

Published 20 hours ago • verified publisher icon ansible-collections

EFS: Add backup option

Open nick-zh opened this issue 2 years ago • 10 comments

Summary

Add the Backup option for EFS

Issue Type

Feature Idea

Component Name

efs

Additional Information

From what i can see here, i assume it should be quite easy to add this i hope. Default should be false

Code of Conduct

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

nick-zh avatar Jun 25 '22 18:06 nick-zh

Files identified in the description:

  • [plugins/modules/efs.py](https://github.com/['ansible-collections/amazon.aws', 'ansible-collections/community.aws', 'ansible-collections/community.vmware']/blob/main/plugins/modules/efs.py)

If these files are inaccurate, please update the component name section of the description or use the !component bot command.

click here for bot help

ansibullbot avatar Jun 29 '22 06:06 ansibullbot

cc @akazakov @jillr @markuman @ryansydnor @s-hertel @tremble click here for bot help

ansibullbot avatar Jun 29 '22 06:06 ansibullbot

@markuman Can I take on this issue?

akamanzi avatar Aug 30 '22 20:08 akamanzi

@markuman Can I take on this issue?

Yes, of course!

markuman avatar Aug 30 '22 20:08 markuman

@markuman @nick-zh ,

Just to confirm that I am doing addressing this issue the right way, the idea is to add a Backup parameter (which is a boolean) create_file_system function (L416) and in the main function

https://github.com/ansible-collections/community.aws/blob/cb9716e14d44357aaadd2be733bbaa0dd8a522bc/plugins/modules/efs.py#L416

akamanzi avatar Sep 02 '22 15:09 akamanzi

@akamanzi yes, exactly. basically you must complete four things.

  1. create_file_system. easy
  2. update_file_system
    • it must be possible to enable/disable the backup feature on existing EFS
    • but take care about backwards compatibility on existing playbooks. therefore, the backup parameter must not have a default value.
  3. expand the integration test
  4. add a changelog fragment to your PR

markuman avatar Sep 02 '22 15:09 markuman

@markuman , Thank you for the guidance. Upon further reading, it looks the update_file_system only updates the throughput (ThroughPutMode) and the amount of provisioned throughput (ProvisionedThroughputInMbs). It seems like the backup is only enabled on creation of the EFS. Check here for reference

akamanzi avatar Sep 02 '22 16:09 akamanzi

@akamanzi ah, I didn't know that. Then it's just step 1, 3 and 4,

markuman avatar Sep 02 '22 17:09 markuman

i still think we need at least the logic of point 2, as it should be possible to enable / disable backup for an EFS if a backup strategy is explicitly set, from what i can see, this can be achieved with put_backup_policy

nick-zh avatar Sep 08 '22 08:09 nick-zh

@nick-zh, I agree we still need logic for point 2 and thanks for pointing this out. The put_backup_policy and describe_backup_policy should do the job. Currently, I have added the backup option while creating the EFS but I had trouble running the integration tests as the documentation is not crystal clear on how run them. However, after searching around I figured it out and should soon add point 2 as well.

akamanzi avatar Sep 08 '22 13:09 akamanzi