Skip to content
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

EFS: Added backup option #1502

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

akamanzi
Copy link

@akamanzi akamanzi commented Sep 19, 2022

SUMMARY

Added backup option to the efs module which enables automatic backup of the file system.
Addresses issue #1266

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

Impacted component: efs module

ADDITIONAL INFORMATION

To enable automatic backup, you can add the following to the playbook

- name: Efs enable automatic backup 
      efs:
        state: present
        name: 'efs-name'
        tags:
            Name: 'tag-name'
        targets:
            - subnet_id: subnet-id
            - subnet_id: subnet-id
        backup: 'enabled'

Disabling automatic backup:

- name: Efs disable automatic backup 
      efs:
        state: present
        name: 'efs-name'
        tags:
            Name: 'tag-name'
        targets:
            - subnet_id: 'subnet-id'
            - subnet_id: 'subnet-id'
        backup: 'disabled'

@github-actions
Copy link

github-actions bot commented Sep 19, 2022

Docs Build 📝

Thank you for contribution!✨

The docsite for this PR is available for download as an artifact from this run:
https://github.com/ansible-collections/community.aws/actions/runs/3085306655

You can compare to the docs for the main branch here:
https://ansible-collections.github.io/community.aws/branch/main

File changes:

  • M collections/community/aws/efs_module.html
Click to see the diff comparison.

NOTE: only file modifications are shown here. New and deleted files are excluded.
See the file list and check the published docs to see those files.

diff --git a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/efs_module.html b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/efs_module.html
index 192ec97..714b458 100644
--- a/home/runner/work/community.aws/community.aws/docsbuild/base/collections/community/aws/efs_module.html
+++ b/home/runner/work/community.aws/community.aws/docsbuild/head/collections/community/aws/efs_module.html
@@ -207,6 +207,19 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </div></td>
 </tr>
 <tr class="row-even"><td><div class="ansible-option-cell">
+<div class="ansibleOptionAnchor" id="parameter-backup"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-backup"><strong>backup</strong></p>
+<a class="ansibleOptionLink" href="#parameter-backup" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
+</div></td>
+<td><div class="ansible-option-cell"><p>Changes the file system backup policy. Use this option to start or stop automatic backups of the file system.</p>
+<p>By default, automatic backup of the file system is disabled.</p>
+<p class="ansible-option-line"><span class="ansible-option-choices">Choices:</span></p>
+<ul class="simple">
+<li><p><span class="ansible-option-choices-entry">enabled</span></p></li>
+<li><p><span class="ansible-option-choices-entry">disabled</span></p></li>
+</ul>
+</div></td>
+</tr>
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-debug_botocore_endpoint_logs"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-debug-botocore-endpoint-logs"><strong>debug_botocore_endpoint_logs</strong></p>
 <a class="ansibleOptionLink" href="#parameter-debug_botocore_endpoint_logs" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -218,7 +231,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-encrypt"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-encrypt"><strong>encrypt</strong></p>
 <a class="ansibleOptionLink" href="#parameter-encrypt" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -230,7 +243,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-endpoint_url"></div>
 <div class="ansibleOptionAnchor" id="parameter-ec2_url"></div>
 <div class="ansibleOptionAnchor" id="parameter-aws_endpoint_url"></div>
@@ -241,28 +254,28 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 <td><div class="ansible-option-cell"><p>URL to use to connect to EC2 or your Eucalyptus cloud (by default the module will use EC2 endpoints). Ignored for modules where region is required. Must be specified for all other modules if region is not used. If not set then the value of the EC2_URL environment variable, if any, is used.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-id"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-id"><strong>id</strong></p>
 <a class="ansibleOptionLink" href="#parameter-id" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>ID of Amazon EFS. Either name or ID required for delete.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-kms_key_id"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-kms-key-id"><strong>kms_key_id</strong></p>
 <a class="ansibleOptionLink" href="#parameter-kms_key_id" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>The id of the AWS KMS CMK that will be used to protect the encrypted file system. This parameter is only required if you want to use a non-default CMK. If this parameter is not specified, the default CMK for Amazon EFS is used. The key id can be Key ID, Key ID ARN, Key Alias or Key Alias ARN.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-name"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-name"><strong>name</strong></p>
 <a class="ansibleOptionLink" href="#parameter-name" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>Creation Token of Amazon EFS file system. Required for create and update. Either name or ID required for delete.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-performance_mode"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-performance-mode"><strong>performance_mode</strong></p>
 <a class="ansibleOptionLink" href="#parameter-performance_mode" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -274,7 +287,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-profile"></div>
 <div class="ansibleOptionAnchor" id="parameter-aws_profile"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-profile"><span id="ansible-collections-community-aws-efs-module-parameter-aws-profile"></span><strong>profile</strong></p>
 <a class="ansibleOptionLink" href="#parameter-profile" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-aliases">aliases: aws_profile</span></p>
@@ -283,14 +296,14 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 <td><div class="ansible-option-cell"><p>The <em>profile</em> option is mutually exclusive with the <em>aws_access_key</em>, <em>aws_secret_key</em> and <em>security_token</em> options.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-provisioned_throughput_in_mibps"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-provisioned-throughput-in-mibps"><strong>provisioned_throughput_in_mibps</strong></p>
 <a class="ansibleOptionLink" href="#parameter-provisioned_throughput_in_mibps" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">float</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>If the throughput_mode is provisioned, select the amount of throughput to provisioned in Mibps.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-purge_tags"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-purge-tags"><strong>purge_tags</strong></p>
 <a class="ansibleOptionLink" href="#parameter-purge_tags" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -304,7 +317,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-region"></div>
 <div class="ansibleOptionAnchor" id="parameter-aws_region"></div>
 <div class="ansibleOptionAnchor" id="parameter-ec2_region"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-region"><span id="ansible-collections-community-aws-efs-module-parameter-ec2-region"></span><span id="ansible-collections-community-aws-efs-module-parameter-aws-region"></span><strong>region</strong></p>
@@ -314,7 +327,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 <td><div class="ansible-option-cell"><p>The AWS region to use. If not specified then the value of the AWS_REGION or EC2_REGION environment variable, if any, is used. See <a class="reference external" href="http://docs.aws.amazon.com/general/latest/gr/rande.html#ec2_region">http://docs.aws.amazon.com/general/latest/gr/rande.html#ec2_region</a></p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-security_token"></div>
 <div class="ansibleOptionAnchor" id="parameter-aws_session_token"></div>
 <div class="ansibleOptionAnchor" id="parameter-session_token"></div>
@@ -328,7 +341,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 <p>Aliases <em>aws_session_token</em> and <em>session_token</em> have been added in version 3.2.0.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-state"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-state"><strong>state</strong></p>
 <a class="ansibleOptionLink" href="#parameter-state" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -340,7 +353,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-tags"></div>
 <div class="ansibleOptionAnchor" id="parameter-resource_tags"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-tags"><span id="ansible-collections-community-aws-efs-module-parameter-resource-tags"></span><strong>tags</strong></p>
 <a class="ansibleOptionLink" href="#parameter-tags" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-aliases">aliases: resource_tags</span></p>
@@ -349,35 +362,35 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 <td><div class="ansible-option-cell"><p>List of tags of Amazon EFS. Should be defined as dictionary In case of ‘present’ state with list of tags and existing EFS (matched by ‘name’), tags of EFS will be replaced with provided data.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-targets"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-targets"><strong>targets</strong></p>
 <a class="ansibleOptionLink" href="#parameter-targets" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=dictionary</span></p>
 </div></td>
 <td><div class="ansible-option-cell"><p>List of mounted targets. It should be a list of dictionaries, every dictionary should include next attributes: This data may be modified for existing EFS using state ‘present’ and new list of mount targets.</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-targets/ip_address"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-targets-ip-address"><strong>ip_address</strong></p>
 <a class="ansibleOptionLink" href="#parameter-targets/ip_address" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
 <td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>A valid IPv4 address within the address range of the specified subnet.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-targets/security_groups"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-targets-security-groups"><strong>security_groups</strong></p>
 <a class="ansibleOptionLink" href="#parameter-targets/security_groups" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">list</span> / <span class="ansible-option-elements">elements=string</span></p>
 </div></td>
 <td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>List of security group IDs, of the form ‘sg-xxxxxxxx’. These must be for the same VPC as subnet specified</p>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-indent"></div><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-targets/subnet_id"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-targets-subnet-id"><strong>subnet_id</strong></p>
 <a class="ansibleOptionLink" href="#parameter-targets/subnet_id" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span> / <span class="ansible-option-required">required</span></p>
 </div></td>
 <td><div class="ansible-option-indent-desc"></div><div class="ansible-option-cell"><p>The ID of the subnet to add the mount target in.</p>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-throughput_mode"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-throughput-mode"><strong>throughput_mode</strong></p>
 <a class="ansibleOptionLink" href="#parameter-throughput_mode" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 </div></td>
@@ -389,7 +402,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-transition_to_ia"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-transition-to-ia"><strong>transition_to_ia</strong></p>
 <a class="ansibleOptionLink" href="#parameter-transition_to_ia" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">string</span></p>
 <p><span class="ansible-option-versionadded">added in community.aws 2.1.0</span></p>
@@ -408,7 +421,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-validate_certs"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-validate-certs"><strong>validate_certs</strong></p>
 <a class="ansibleOptionLink" href="#parameter-validate_certs" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -420,7 +433,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-odd"><td><div class="ansible-option-cell">
+<tr class="row-even"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-wait"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-wait"><strong>wait</strong></p>
 <a class="ansibleOptionLink" href="#parameter-wait" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">boolean</span></p>
 </div></td>
@@ -432,7 +445,7 @@ see <a class="reference internal" href="#ansible-collections-community-aws-efs-m
 </ul>
 </div></td>
 </tr>
-<tr class="row-even"><td><div class="ansible-option-cell">
+<tr class="row-odd"><td><div class="ansible-option-cell">
 <div class="ansibleOptionAnchor" id="parameter-wait_timeout"></div><p class="ansible-option-title" id="ansible-collections-community-aws-efs-module-parameter-wait-timeout"><strong>wait_timeout</strong></p>
 <a class="ansibleOptionLink" href="#parameter-wait_timeout" title="Permalink to this option"></a><p class="ansible-option-type-line"><span class="ansible-option-type">integer</span></p>
 </div></td>

@softwarefactory-project-zuul
Copy link
Contributor

Build failed.

✔️ ansible-galaxy-importer SUCCESS in 3m 34s
✔️ build-ansible-collection SUCCESS in 4m 57s
✔️ ansible-test-sanity-docker-devel SUCCESS in 10m 12s (non-voting)
✔️ ansible-test-sanity-docker-milestone SUCCESS in 9m 10s
✔️ ansible-test-sanity-docker-stable-2.12 SUCCESS in 9m 31s
✔️ ansible-test-sanity-docker-stable-2.13 SUCCESS in 9m 43s
✔️ ansible-test-units-community-aws-python38 SUCCESS in 6m 30s
✔️ ansible-test-units-community-aws-python39 SUCCESS in 6m 28s
✔️ ansible-test-splitter SUCCESS in 2m 43s
integration-community.aws-1 FAILURE in 12m 21s
⚠️ integration-community.aws-2 SKIPPED
⚠️ integration-community.aws-3 SKIPPED
⚠️ integration-community.aws-4 SKIPPED
⚠️ integration-community.aws-5 SKIPPED
⚠️ integration-community.aws-6 SKIPPED
⚠️ integration-community.aws-7 SKIPPED
⚠️ integration-community.aws-8 SKIPPED
⚠️ integration-community.aws-9 SKIPPED
⚠️ integration-community.aws-10 SKIPPED
⚠️ integration-community.aws-11 SKIPPED
⚠️ integration-community.aws-12 SKIPPED
⚠️ integration-community.aws-13 SKIPPED
⚠️ integration-community.aws-14 SKIPPED
⚠️ integration-community.aws-15 SKIPPED
⚠️ integration-community.aws-16 SKIPPED
⚠️ integration-community.aws-17 SKIPPED
⚠️ integration-community.aws-18 SKIPPED

@ansibullbot
Copy link

@ansibullbot ansibullbot added community_review feature This issue/PR relates to a feature request integration tests/integration module module needs_triage new_contributor Help guide this first time contributor plugins plugin (any type) tests tests labels Sep 19, 2022
@akamanzi
Copy link
Author

@markuman, this feature requires elasticfilesystem:PutBackupPolicy and elasticfilesystem:DescribeBackupPolicy privileges on AWS

@markuman
Copy link
Member

@akamanzi cool to see your PR.
Permissions are handled in a different project for community.aws

https://github.com/mattclay/aws-terminator/blob/master/aws/policy/storage-services.yaml

That means you must made a 2nd pull request there and reference to your PR here.

Unfortunately I'm out of office until end of month, maybe @alinabuzachis can assist you there.

@akamanzi
Copy link
Author

@markuman, thanks for getting back.

@alinabuzachis, I made the second PR to the aws-terminator (mattclay/aws-terminator#228) adding the required policies.

Copy link
Member

@goneri goneri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @akamanzi for the PR.

plugins/modules/efs.py Show resolved Hide resolved
@@ -79,6 +79,12 @@
description:
- If the throughput_mode is provisioned, select the amount of throughput to provisioned in Mibps.
type: float
backup:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you call it backup_policy to be consistent with AWS's naming.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense, I will change this

@@ -274,6 +280,7 @@ class EFSConnection(object):
STATE_AVAILABLE = 'available'
STATE_DELETING = 'deleting'
STATE_DELETED = 'deleted'
BACKUP_DISABLED = 'DISABLED'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure the constant increase the readability.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would BACKUP_POLICY_DISABLED or BACKUP_POLICY_STATUS_DISABLED improve on readability?

"""
changed = False
state = self.get_file_system_state(name)
if state in [self.STATE_AVAILABLE, self.STATE_CREATING]:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If state doesn't match the condition, we silently ignore it and return "unchanged" even if the value is not the one expected.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand what you mean by this, isn't the "unchanged" when the changed= False?

)
fs_id = self.get_file_system_id(name)
current_backup_policy = self.get_backup_policy(fs_id)
params = dict()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is valide, but params in general is already used for the module parameters. Can you use a different name for your variable?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure

@@ -767,6 +818,9 @@ def main():
throughput_mode=throughput_mode, provisioned_throughput_in_mibps=provisioned_throughput_in_mibps) or changed
if transition_to_ia:
changed |= connection.update_lifecycle_policy(name, transition_to_ia)
if backup:
backup = str(backup).upper()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

backup is already a string, you don't need the str() cast.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this catch

@@ -767,6 +818,9 @@ def main():
throughput_mode=throughput_mode, provisioned_throughput_in_mibps=provisioned_throughput_in_mibps) or changed
if transition_to_ia:
changed |= connection.update_lifecycle_policy(name, transition_to_ia)
if backup:
backup = str(backup).upper()
changed = connection.change_backup_policy(name, backup)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should changed |=, not just =, this way we preserve the value if it's set True above.

e.g:

>>> changed = False
>>> changed |= False
>>> changed |= True
>>> changed |= False
>>> changed
True

abikouo pushed a commit to abikouo/community.aws that referenced this pull request Oct 24, 2023
…error occurred (AccessControlListNotSupported) when calling the PutObject operation: The bucket does not allow ACLs (ansible-collections#1502)

Comment on permission because of botocore.exceptions.ClientError: An error occurred (AccessControlListNotSupported) when calling the PutObject operation: The bucket does not allow ACLs

SUMMARY

Comment on permission because of botocore.exceptions.ClientError: An error occurred (AccessControlListNotSupported) when calling the PutObject operation: The bucket does not allow ACLs

ISSUE TYPE


Bugfix Pull Request
Docs Pull Request
Feature Pull Request
New Module Pull Request

COMPONENT NAME

ADDITIONAL INFORMATION

Reviewed-by: Jill R
Reviewed-by: Alina Buzachis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review feature This issue/PR relates to a feature request integration tests/integration module module new_contributor Help guide this first time contributor plugins plugin (any type) tests tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants