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

Adding support for managing AWS IOT things #32826

Open
wants to merge 11 commits into
base: devel
from

Conversation

@miokowpak
Copy link

commented Nov 12, 2017

SUMMARY

Add support for managing AWS IOT things. When ansible is used to provision an IOT device, it is convenient to let it set up the state required in the cloud.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

iot_thing

ANSIBLE VERSION
ansible 2.5.0 (iot_thing 4f6dac2c01) last updated 2017/11/12 12:23:35 (GMT -400)
  config file = /etc/ansible/ansible.cfg
  configured module search path = [u'/home/akowpak/dev/ansible/library']
  ansible python module location = /home/akowpak/dev/ansible/lib/ansible
  executable location = /home/akowpak/dev/ansible/bin/ansible
  python version = 2.7.6 (default, Jun 22 2015, 17:58:13) [GCC 4.8.2]
ADDITIONAL INFORMATION

N/A

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2017

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2017

@Constantin007 @Constantin07 @Deepakkothandan @Etherdaemon @Java1Guy @Lujeni @MichaelBaydoun @Sodki @adq @akazakov @alachaum @amir343 @anryko @bekelchik @bpennypacker @brandond @carsongee @defunctio @dkhenry @fiunchinho @garethr @gunzy83 @gurumaia @hyperized @infectsoldier @j-carl @jarv @Java1Guy @jimbydamonk @jmenga @joelthompson @jonhadfield @jsdalton @jsmartin @kaczynskid @leedm777 @linuxdynasty @loia @lwade @MichaelBaydoun @michaeljs1990 @minichate @mjschultz @mmochan @nadirollo @nand0p @naslanidis @NickBall @pjodouin @prasadkatti @psykotox @pwnall @raags @rickmendes @roadmapper @ryansydnor @scicoin-project @scottanderson42 @shepdelacreme @silviud @simplesteph @steynovich @tastychutney @tedder @tgerla @timmahoney @tombamford @whiter @wilvk @wimnat @zacblazic @zbal @zeekin @zimbatm

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@willthames
Copy link
Contributor

left a comment

This module should be called aws_iot_thing

lib/ansible/modules/cloud/amazon/iot_thing.py Outdated
HAS_BOTO3 = False


def main():

This comment has been minimized.

Copy link
@willthames

willthames Nov 15, 2017

Contributor

All other modules have main at the bottom - having it here is a little unexpected

lib/ansible/modules/cloud/amazon/iot_thing.py Outdated
argument_spec = dict(
name=dict(required=True, type='str'),
state=dict(default='present', choices=['present', 'absent']),
type=dict(type='str'),

This comment has been minimized.

Copy link
@willthames

willthames Nov 15, 2017

Contributor

type='str' is the default and thus not required

@willthames
Copy link
Contributor

left a comment

It's probably worth adding integration tests which tend to catch a bunch of things that unit tests don't

lib/ansible/modules/cloud/amazon/iot_thing.py Outdated
def change_thing_in_cloud(aws_module, client, update_args):
try:
client.update_thing(**update_args)
except ClientError as e:

This comment has been minimized.

Copy link
@willthames

willthames Nov 15, 2017

Contributor

You need to catch BotoCoreError as well for all AWS API calls.

lib/ansible/modules/cloud/amazon/iot_thing.py Outdated
thing_name = aws_module.params.get('name')

region, ec2_url, aws_connect_kwargs = get_aws_connection_info(aws_module, boto3=True)
if not region:

This comment has been minimized.

Copy link
@willthames

willthames Nov 15, 2017

Contributor

This should no longer be required - boto3_conn will catch a NoRegionError and fail there.

lib/ansible/modules/cloud/amazon/iot_thing.py Outdated
region=region,
endpoint=ec2_url,
**aws_connect_kwargs)
except (ClientError, ValidationError) as e:

This comment has been minimized.

Copy link
@willthames

willthames Nov 15, 2017

Contributor

These errors never happen - there are no reasons to add exception handling around boto3_conn, which is designed to turn all exceptions into useful failure messages

@miokowpak

This comment has been minimized.

Copy link
Author

commented Nov 15, 2017

Thanks for the feedback. I'll make the requested changes as soon as I have a free moment (likely this weekend).

@willthames

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

@akowpak-miovision no worries - it looks to me to be a well written module in general, almost entirely in compliance with latest standards - certainly a great start.

lib/ansible/modules/cloud/amazon/iot_thing.py Outdated
#!/usr/bin/python
# This file is part of Ansible
#
# Ansible is free software: you can redistribute it and/or modify

This comment has been minimized.

lib/ansible/modules/cloud/amazon/iot_thing.py Outdated
returned: on success when I(state=present)
type: int
sample: 123
attribute:

This comment has been minimized.

@miokowpak miokowpak force-pushed the miokowpak:iot_thing branch 4 times, most recently Nov 19, 2017

@miokowpak

This comment has been minimized.

Copy link
Author

commented Nov 19, 2017

@mattclay @gundalow @willthames: I assumed that in order to get my integration tests to run on shippable, I'd have to setup a policy in the testing_policies directory, as I have done. I've tried many iterations restricting the resources, all the way to what I have in my commit allowing all resources, and none of that works.

If you can point me in the correct direction of what I need to do to get permissions set up, I would appreciate it. For what it's worth, this PR is only one of four modules I intend to contribute, so, when I push the other 3 IoT modules, I'll require more IoT permissions.

Thanks very much for any help you can provide.

@mattclay

This comment has been minimized.

Copy link
Member

commented Nov 20, 2017

@akowpak-miovision The test policies in the repo are for manual testing. Our CI system has separate policies which need to be updated. That's something @s-hertel or myself can take care of.

@miokowpak

This comment has been minimized.

Copy link
Author

commented Nov 20, 2017

Thanks @mattclay. If you guys can let me know when the policies have been updated, it would be most appreciated.

@miokowpak miokowpak force-pushed the miokowpak:iot_thing branch Nov 22, 2017

@ansibot ansibot added the stale_ci label Dec 1, 2017

@ansibot ansibot added new_contributor and removed stale_ci labels Jan 23, 2018

@s-hertel s-hertel force-pushed the miokowpak:iot_thing branch to e6e418b Jan 30, 2018

- name: set the type associated with an IOT thing
aws_iot_thing:
name: mything
remove_type: True

This comment has been minimized.

Copy link
@ryansb

ryansb Feb 1, 2018

Contributor

I'd rather have this either:

a) be called purge_type as we have in other modules called similar features purge_tags etc
b) distinguish between the user providing None and not providing a value, so if they wish to clear the type they can pass type: null or similar.

- name: attach a certificate to an IOT thing
aws_iot_thing:
name: mything
attached_principals:

This comment has been minimized.

Copy link
@ryansb

ryansb Feb 1, 2018

Contributor

To me this looks like it should be principals and then have a purge_principals option to remove any principals not explicitly listed.

- name: remove attributes from an IOT thing
aws_iot_thing:
name: mything
absent_attributes:

This comment has been minimized.

Copy link
@ryansb

ryansb Feb 1, 2018

Contributor

Same: see purge_* options on iam_role and other modules.

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.