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

bugfix. cloudwatchlogs_log_group module wrongly assumes all log group… #52513

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@DaveHewy
Copy link

DaveHewy commented Feb 18, 2019

…s will have retentionInDays present in dict. not true, so patched.
AWS does not return the key in describe operations if the value has not been set in a previous create (input_retention_policy). Additionally if the user has set a retention param value and the retentionInDays key is absent then the module should allow one to be set (which it does not).

SUMMARY

cloudwatchlogs_log_group wrongly assumes a dict element of 'retentionInDays' will be present on describe operation. this update allows for an element without retentionInDays to be updated without KeyError.

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

cloudwatchlogs_log_group

ADDITIONAL INFORMATION

To reproduce

    - aws_cloudwatchlogs_log_group:
        state: present
        log_group_name: log_group_example

then re-run a playbook with the following:

    - aws_cloudwatchlogs_log_group:
        state: present
        log_group_name: log_group_example
        retention: 365

You will get KeyError

can provide more info if needed.

David Heward
bugfix. cloudwatchlogs_log_group module wrongly assumes all log group…
…s will have retentionInDays present in dict. not true, so patched
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 18, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Feb 18, 2019

@DaveHewy, just so you are aware we have a dedicated Working Group for aws.
You can find other people interested in this in #ansible-aws on Freenode IRC
For more information about communities, meetings and agendas see https://github.com/ansible/community

click here for bot help

@DaveHewy

This comment has been minimized.

Copy link
Author

DaveHewy commented Feb 22, 2019

@ansibot any movement on this?

@nathanwebsterdotme

This comment has been minimized.

Copy link
Contributor

nathanwebsterdotme commented Feb 28, 2019

👍

@ansibot ansibot added the stale_ci label Feb 28, 2019

@gundalow

This comment has been minimized.

Copy link
Contributor

gundalow commented Mar 11, 2019

@nathanwebsterdotme Have you had the chance to test this, does this patch work for you/

@ansibot ansibot removed the needs_triage label Mar 11, 2019

@@ -259,12 +259,15 @@ def main():
retention=module.params['retention'],
module=module)
elif found_log_group:
if module.params['retention'] != found_log_group['retentionInDays']:

This comment has been minimized.

@willthames

willthames Mar 12, 2019

Contributor

A simpler fix would be

if module.params['retention'] != found_log_group.get('retentionInDays'):

wouldn't it?

I'm not at all convinced of the need to reformat the call to input_retention_policy - PEP8 does not express a preference either way: https://www.python.org/dev/peps/pep-0008/#id17.

This comment has been minimized.

@gundalow

gundalow Mar 18, 2019

Contributor

Our coding style is "Whatever is enforced by CI"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.