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

feat(ec2.py): enhance regions input #46666

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
5 participants
@hanks

hanks commented Oct 9, 2018

SUMMARY

Enhance regions input like below:

  1. add --regions option
    currently there is no option for regions, so add this for handy
  2. support space and comma for region input
    because in ec2.ini, input for regions_exclude uses comma and space, but regions can not, it is a little confused, Add this to make them consistent
  3. fix logic to always exclude regions specified
    seems regions_exclude does not be used for all the conditions, so add logic to exclude every time
  4. fix logic to call eucalyptus, there is no usage like self.regions.append(boto.connect_euca(host=self.eucalyptus_host).region.name, **self.credentials), shoule be self.regions.append(boto.connect_euca(host=self.eucalyptus_host, **self.credentials).region.name)
  5. add unittest for regions part in ec2.py
  6. add docs for options usage
  7. refactor a little for unit test easily
ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

contrib/inventory/ec2.py

ANSIBLE VERSION
ansible 2.7.0
  config file = None
  configured module search path = ['/Users/hanzhou/.ansible/plugins/modules', '/usr/share/ansible/plugins/modules']
  ansible python module location = /usr/local/var/pyenv/versions/3.6.4/envs/3.6.4/lib/python3.6/site-packages/ansible
  executable location = /usr/local/var/pyenv/versions/3.6.4/bin/ansible
  python version = 3.6.4 (default, Mar 23 2018, 19:03:30) [GCC 4.2.1 Compatible Apple LLVM 9.0.0 (clang-900.0.39.2)]
@ansibot

This comment has been minimized.

Contributor

ansibot commented Oct 9, 2018

@hanks: thank you for submitting this pull-request !

cc @ryansb @s-hertel @willthames
click here for bot help

import os
import sys
import unittest
from unittest.mock import MagicMock, patch

This comment has been minimized.

@mattclay

mattclay Oct 9, 2018

Member

New unit tests should be written to use pytest instead of unittest.

@mattclay

This comment has been minimized.

Member

mattclay commented Oct 9, 2018

@mattclay mattclay added the ci_verified label Oct 9, 2018

@hanks

This comment has been minimized.

hanks commented Oct 9, 2018

@mattclay thank you for the review

ok, I will use pytest, and for the unittest error, because I use the built in libs in python3.
Do I need to make the test run both py3 and py2, or just py3?

Thanks for your time

@mattclay

This comment has been minimized.

Member

mattclay commented Oct 9, 2018

@hanks The unit tests need to run on Python versions 2.6, 2.7, 3.5, 3.6 and 3.7 currently.

@hanks hanks force-pushed the hanks:feat/add-regions-args-to-ec2-py branch from 131fe9f to a9b00b8 Oct 10, 2018

@ansibot ansibot added test and removed ci_verified labels Oct 10, 2018

@hanks hanks force-pushed the hanks:feat/add-regions-args-to-ec2-py branch from a9b00b8 to 13bac01 Oct 10, 2018

@hanks

This comment has been minimized.

hanks commented Oct 10, 2018

@mattclay I changed to pytest follow docs from https://docs.ansible.com/ansible/devel/dev_guide/testing_units.html, Please check.

And one issue is that I met like below:

08:33 ------------------------------- Captured stderr --------------------------------
08:33 Failed to import docker-py. Try `pip install docker-py` - cannot import name 'APIClient' from 'docker' (/root/ansible/contrib/inventory/docker.py)
08:33 __ ERROR collecting test/units/modules/cloud/docker/test_docker_container.py ___
08:33 test/units/modules/cloud/docker/test_docker_container.py:3: in <module>
08:33     from ansible.modules.cloud.docker.docker_container import TaskParameters
08:33 lib/ansible/modules/cloud/docker/docker_container.py:710: in <module>
08:33     from ansible.module_utils.docker_common import (
08:33 lib/ansible/module_utils/docker_common.py:34: in <module>
08:33     from docker import __version__ as docker_version
08:33 contrib/inventory/docker.py:893: in <module>
08:33     main()
08:33 contrib/inventory/docker.py:888: in main
08:33     fail("Failed to import docker-py. Try `pip install docker-py` - %s" % (HAS_DOCKER_ERROR))
08:33 contrib/inventory/docker.py:422: in fail
08:33     sys.exit(1)
08:33 E   SystemExit: 1

seems there are not enough unit tests for crontrib/inventory, and when run my test code, it will import the top-level docker-py from crontrib/inventory/docker.py, So I also added into units requirements.txt

@hanks hanks force-pushed the hanks:feat/add-regions-args-to-ec2-py branch from 6af8b9d to 34569ca Oct 10, 2018

@ansibot ansibot removed the needs_revision label Oct 10, 2018

os.path.dirname(os.path.dirname(os.path.abspath(__file__))))))
inventory_dir = os.path.join(root_dir, 'contrib', 'inventory')
inventory_dir_abspath = os.path.abspath(inventory_dir)
sys.path.append(inventory_dir_abspath)

This comment has been minimized.

@mattclay

mattclay Oct 10, 2018

Member

It's not safe to modify sys.path outside of unit test functions, as these changes will affect other unrelated tests.

This comment has been minimized.

@mattclay

mattclay Oct 10, 2018

Member

You may be able to modify sys.path, perform an import, and then restore sys.path. That will probably prevent it from causing issues for other unit tests, but I haven't verified that.

This comment has been minimized.

@hanks

hanks Oct 10, 2018

now I use teardown_module to restore sys.path, and as you mentioned, it seems to be not safe, so fix like below:

sys.path.append(inventory_dir_abspath)
from ec2 import Ec2Inventory
# cleanup so that path is not polluted with other scripts
sys.path.remove(inventory_dir_abspath)

This comment has been minimized.

@mattclay

mattclay Oct 10, 2018

Member

I found a better way to handle the import, using a pytest fixture.

I have a PR (#46732) to fix the vmware_inventory unit tests that provides a good example:

https://github.com/ansible/ansible/pull/46732/files#diff-7a31f0c775c6683a8212d1f175db50e9R28

The BaseException handling was only needed because the inventory script uses sys.exit for import failures.

This comment has been minimized.

@hanks

hanks Oct 10, 2018

cool! fixed as your way

@hanks hanks force-pushed the hanks:feat/add-regions-args-to-ec2-py branch from 34569ca to e2bff64 Oct 10, 2018

boto3
docker-py

This comment has been minimized.

@mattclay

mattclay Oct 10, 2018

Member

This import shouldn't be needed with the sys.path being cleaned up.

'ec2.boto.connect_euca', mocker.MagicMock(return_value=mock1))
class TestEC2Inventory(object):

This comment has been minimized.

@mattclay

mattclay Oct 10, 2018

Member

When using pytest, create top-level functions without using a class.

@hanks hanks force-pushed the hanks:feat/add-regions-args-to-ec2-py branch 4 times, most recently from 8c3f13d to 7298322 Oct 10, 2018

try:
import ec2
except ImportError as ex:
pytest.fail(ex)

This comment has been minimized.

@mattclay

mattclay Oct 10, 2018

Member

Since only ImportError is being handled here, use this instead:

try:
    ec2 = pytest.importorskip("ec2")
finally:
    # rest of code here

That way the tests will be properly skipped if the import isn't available.

This comment has been minimized.

@hanks

hanks Oct 10, 2018

fixed it, so the policy is like to skip test if ImportError happens, and do not fail it?

This comment has been minimized.

@mattclay

mattclay Oct 10, 2018

Member

Correct.

@hanks hanks force-pushed the hanks:feat/add-regions-args-to-ec2-py branch 4 times, most recently from 5c9125b to 11e709a Oct 10, 2018

feat(ec2.py): enhance regions input
1. add --regions option
2. support space and comma for region input
3. fix logic to always exclude regions specified
4. fix logic to call eucalyptus
5. add unittest for regions part in ec2.py
6. add docs for options usage
7. refactor a little for unit test easily

@hanks hanks force-pushed the hanks:feat/add-regions-args-to-ec2-py branch from 11e709a to d7ede84 Oct 10, 2018

@hanks

This comment has been minimized.

hanks commented Oct 24, 2018

@mattclay Hi, is there anything improvement I can do for this PR?

@hanks hanks closed this Oct 25, 2018

@hanks hanks reopened this Oct 25, 2018

@ansibot ansibot removed the stale_ci label Oct 25, 2018

@ansibot ansibot added the stale_ci label Nov 5, 2018

@@ -66,6 +66,7 @@ coverage.xml
/test/units/cover-html
/test/integration/targets/*/backup/
/test/cache/*
metadata-*.json

This comment has been minimized.

@mattclay

mattclay Nov 7, 2018

Member

This change should be made in a separate PR since it's not directly related to the rest of the changes here.

@mattclay

This comment has been minimized.

Member

mattclay commented Nov 7, 2018

@hanks Thanks for including tests with your changes. Have you tried finding someone in the Ansible community that uses this inventory script to test and review your changes? I recommend asking in the #ansible-aws IRC channel on Freenode.

@ansibot ansibot removed the stale_ci label Nov 26, 2018

@s-hertel

This comment has been minimized.

Contributor

s-hertel commented Nov 30, 2018

Hi @hanks, nice that you included tests for this. 😄 We aren't working on adding features to the contrib scripts since we have added inventory plugins. The aws_ec2 inventory plugin uses boto3, has more flexible group and hostvar configuration, has common caching infrastructure, and uses configuration files rather than passing command line arguments. Have you looked into the aws_ec2 inventory plugin to see if it meets your needs? https://docs.ansible.com/ansible/latest/plugins/inventory.html#using-inventory-plugins

If it does not, submitting changes to modify https://github.com/ansible/ansible/blob/devel/lib/ansible/plugins/inventory/aws_ec2.py is a better way to make changes to EC2 inventory at this point. However, since bugfixes may still be added to contrib/inventory/ec2.py, having unit tests would be really nice. Do you think there's a point to separating tests from your changes?

@ansibot ansibot added the stale_ci label Dec 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment