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

[Docs] Updated rds_instance backup_retention_period parameter docs #183

Merged
merged 1 commit into from
Aug 12, 2020
Merged

[Docs] Updated rds_instance backup_retention_period parameter docs #183

merged 1 commit into from
Aug 12, 2020

Conversation

JeanMarcSaad
Copy link
Contributor

SUMMARY

Documentation update:
When using rds_instance module, I noticed that when the backup_retention_period parameter is set to 0,
automated backups are disabled.
Previous documentation mentioned that this parameter should be greater than or equal to 1.
Documentation has been updated to reflect this behavior.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

rds_instance

Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

A couple of minor formatting niggles. But the change seems sound based on the AWS documentation.

It's possible that when this module was first created it was required to be >1 (and has to be for DBs that will had a read-replica attached to them)

@@ -99,7 +99,8 @@
type: str
backup_retention_period:
description:
- The number of days for which automated backups are retained (must be greater or equal to 1).
- The number of days for which automated backups are retained (>=1).
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- The number of days for which automated backups are retained (>=1).
- The number of days for which automated backups are retained.

@@ -99,7 +99,8 @@
type: str
backup_retention_period:
description:
- The number of days for which automated backups are retained (must be greater or equal to 1).
- The number of days for which automated backups are retained (>=1).
When set to 0, automated backups will be disabled.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
When set to 0, automated backups will be disabled.
- When set to C(0), automated backups will be disabled.

Part of me thinks we should also mention the caveat that Can't be set to 0 if the DB instance is a source to read replicas ( https://boto3.amazonaws.com/v1/documentation/api/latest/reference/services/rds.html#RDS.Client.create_db_instance )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We definitely should mention the read replicas case! Why the C(0) btw?

Copy link
Contributor

Choose a reason for hiding this comment

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

Using C() tweaks the formatting a little and formats it as a 'value'. See also:
https://docs.ansible.com/ansible/latest/dev_guide/developing_modules_documenting.html#linking-within-module-documentation

@@ -99,7 +99,8 @@
type: str
backup_retention_period:
description:
- The number of days for which automated backups are retained (must be greater or equal to 1).
- The number of days for which automated backups are retained (>=1).
When set to 0, automated backups will be disabled.
May be used when creating a new cluster, when restoring from S3, or when modifying a cluster.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
May be used when creating a new cluster, when restoring from S3, or when modifying a cluster.
- May be used when creating a new cluster, when restoring from S3, or when modifying a cluster.

@JeanMarcSaad
Copy link
Contributor Author

Fully aligned 🙂 Will be updating the pull request now!

@JeanMarcSaad JeanMarcSaad requested a review from tremble August 12, 2020 19:26
Copy link
Contributor

@tremble tremble left a comment

Choose a reason for hiding this comment

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

LGTM

@jillr is in the middle of preparing the 1.1.0 release. I think it would be good to get this included, but I don't want to merge and cause problems.

@tremble tremble merged commit 11a87e0 into ansible-collections:main Aug 12, 2020
@JeanMarcSaad JeanMarcSaad deleted the update-rds-backup-retention-docs branch August 20, 2020 11:01
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants