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

Use lower case days in rds_instance's "preferred_maintenance_window" parameter docs #310

Closed

Conversation

dnmvisser
Copy link

SUMMARY

The current docs explicitly list the days with capitals (Mon, Tue) etc. This works, but AWS then stores this as lowercase, causing idempotency issues the next time the task is run as the string value is different.

ISSUE TYPE
  • Docs Pull Request
COMPONENT NAME

rds_instance

ADDITIONAL INFORMATION

The integration test actually do use the all lowercase format:
https://github.com/ansible-collections/community.aws/blob/main/tests/integration/targets/rds_instance/tasks/test_modification.yml#L152

NOTE: it looks as if other modules have a similar documentation issue, but I didn't confirm this:

https://github.com/ansible-collections/community.aws/blob/main/plugins/modules/rds.py#L137
https://github.com/ansible-collections/community.aws/blob/main/plugins/modules/redshift.py#L103

Copy link
Member

@markuman markuman left a comment

Choose a reason for hiding this comment

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

I wonder if the module than should do a lowercase transformation, even if uppercase was requested.
What do you think? @tremble @jillr

@tremble
Copy link
Contributor

tremble commented Dec 7, 2020

@markuman I agree, tweaking the documentation feels like the wrong solution.

@ansibullbot
Copy link

@tremble
Copy link
Contributor

tremble commented Apr 1, 2021

@dnmvisser Thanks for taking the time to submit this PR.

This feels like the wrong solution: case shouldn't matter, it's too easy to get wrong and Amazon doesn't care about the case. I've started on #516 (the integration tests are currently a little broken) and will fix the underlying issue rather than papering-over the issue by changing the docs.

@tremble tremble closed this Apr 1, 2021
alinabuzachis pushed a commit to alinabuzachis/community.aws that referenced this pull request May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community_review docs module module new_contributor Help guide this first time contributor plugins plugin (any type) small_patch Hopefully easy to review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants