Skip to content

Conversation

@rsdcobalt
Copy link
Contributor

There have been some changes with ansible 2.8 that aren't compatible with 2.7. I've added a check to handle those differences.

@rsdcobalt rsdcobalt mentioned this pull request May 24, 2019
Copy link

@dabcoder dabcoder left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this, much appreciated. Left a quick note.

import logging
import os
import time
from packaging import version

Choose a reason for hiding this comment

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

This resulted in having the callback ignored if packaging is not installed (with pip) when I tested this.
Do you think we should include it in the list of requirements then (with datadogpy and pyyaml)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it should be there. Just pushed a commit to put it in the README.

Copy link
Member

@hush-hush hush-hush left a comment

Choose a reason for hiding this comment

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

Hi @rsdcobalt, thanks for the PR !

I added a few comments, tell me what you think.

from ansible.plugins.callback import CallbackBase
from __main__ import cli

ANSIBLE_NEW = False
Copy link
Member

Choose a reason for hiding this comment

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

Could you rename this to ANSIBLE_ABOVE_28 or something similar as it will not always be the "new" version ?

self._playbook_name, _ = os.path.splitext(
os.path.basename(playbook_file_name))
if isinstance(inventory, list):
if isinstance(inventory, list) or isinstance(inventory, tuple):
Copy link
Member

Choose a reason for hiding this comment

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

This could be on condition: if isinstance(inventory, (list, tuple)):

import logging
import os
import time
from packaging import version
Copy link
Member

Choose a reason for hiding this comment

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

Could you move this line within the try/catch line 9 and update the error message line 32 ?

@rsdcobalt
Copy link
Contributor Author

@hush-hush yeah, those make sense. I just committed the changes.

@hush-hush
Copy link
Member

Thanks a lot for this PR @rsdcobalt, merging today !

@hush-hush hush-hush merged commit f70c645 into DataDog:master May 31, 2019
@rsdcobalt rsdcobalt deleted the ansible-2.8-compatibility branch May 31, 2019 17:27
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.

3 participants