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

Add new module to manage SmartOS images through imgadm(1M) #19696

Merged
merged 5 commits into from Jan 5, 2017

Conversation

Projects
None yet
6 participants
@jasperla
Contributor

jasperla commented Dec 27, 2016

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

imgadm

ANSIBLE VERSION
ansible 2.3.0 (imgadm 56b1ba6b7d) last updated 2016/12/27 17:41:59 (GMT +200)
  config file =
  configured module search path = Default w/o overrides
SUMMARY

This new module allows Ansible to manage SmartOS images through imgadm(1M). While many tools on SmartOS take JSON for input, or emit it as output, unfortunately this cannot be used (here) as the JSON output from imgadm(1M) results in errors of the JSON parsers ("invalid object") due to newlines. At least for now it's not strictly needed.

tasks:

  - name: add new source
    imgadm:
      source: 'https://datasets.project-fifo.net'
      state: present

  - name: delete a source
    imgadm:
      source: 'https://datasets.joyent.com/datasets'
      state: absent

  - name: import image
    imgadm:
      uuid: 'e7b557c2-b2a8-11e6-b2c1-7f681aa61371'
      state: imported

  - name: delete image
    imgadm:
      uuid: 'e7b557c2-b2a8-11e6-b2c1-7f681aa61371'
      state: deleted

  - name: update installed images
    imgadm:
      uuid: '*'
      state: updated

output:

TASK [add new source] **********************************************************
ok: [calafate-gz] => {"changed": false, "source": "https://datasets.project-fifo.net", "state": "present"}

TASK [delete a source] *********************************************************
ok: [calafate-gz] => {"changed": false, "source": "https://datasets.joyent.com/datasets", "state": "absent"}

TASK [import image] ************************************************************
ok: [calafate-gz] => {"changed": false, "state": "imported", "uuid": "e7b557c2-b2a8-11e6-b2c1-7f681aa61371"}

TASK [delete image] ************************************************************
changed: [calafate-gz] => {"changed": true, "state": "deleted", "uuid": "e7b557c2-b2a8-11e6-b2c1-7f681aa61371"}

TASK [update installed images] *************************************************
changed: [calafate-gz] => {"changed": true, "state": "updated", "uuid": "*"}
@jasperla

This comment has been minimized.

Contributor

jasperla commented Jan 3, 2017

Updated to address an issue with multiline error messages. I'd appreciate feedback or even a merge.

@gundalow

needs_revision

lib/ansible/modules/cloud/smartos/imgadm.py Outdated
source:
required: false
description:
- URI for the image source

This comment has been minimized.

@gundalow

gundalow Jan 3, 2017

Contributor

Missing full stop. All descriptions: must be full sentences (capital letter & full stop)

This comment has been minimized.

@jasperla

jasperla Jan 3, 2017

Contributor

Fixed.

lib/ansible/modules/cloud/smartos/imgadm.py Outdated
uuid:
required: false
description:
- Image UUID. Can either be a full UUID or '*'.

This comment has been minimized.

@gundalow

gundalow Jan 3, 2017

Contributor

To format this a little better I'd suggest using C(..), also explain what * means.
Image UUID. Can either be a full UUID or C(*) for FIXME.

This comment has been minimized.

@jasperla

jasperla Jan 3, 2017

Contributor

Fixed.

lib/ansible/modules/cloud/smartos/imgadm.py Outdated
'''
EXAMPLES = '''
# Import an image

This comment has been minimized.

@gundalow

gundalow Jan 3, 2017

Contributor

Please rewrite the examples in multi-line YAML with key: value
Also use set the -name: rather than putting that in a comment

This comment has been minimized.

@jasperla

jasperla Jan 3, 2017

Contributor

Fixed.

lib/ansible/modules/cloud/smartos/imgadm.py Outdated
module.exit_json(**result)
from ansible.module_utils.basic import *

This comment has been minimized.

@gundalow

gundalow Jan 3, 2017

Contributor

Please only import what you need, rather than *

This comment has been minimized.

@jasperla

jasperla Jan 3, 2017

Contributor

Fixed.

lib/ansible/modules/cloud/smartos/imgadm.py Outdated
type=dict(default='imgapi', choices=['imgapi', 'docker', 'dsapi']),
uuid=dict(default=None)
),
supports_check_mode=False,

This comment has been minimized.

@gundalow

gundalow Jan 3, 2017

Contributor

What would be needed to make this support check mode?

This comment has been minimized.

@jasperla

jasperla Jan 3, 2017

Contributor

This module relies largely on imgadm(1M) to enforce idempotency, which does not provide a "noop" (or equivalent) mode to do a dry-run. Therefore adding check mode would require major changes to this module. It is something that could be added at some point though.

This comment has been minimized.

@gundalow

gundalow Jan 4, 2017

Contributor

Sounds reasonable. Could you please add that as a note in the code next to the supports_check_mode=False, section

@jasperla jasperla force-pushed the jasperla:imgadm branch to d23e19f Jan 3, 2017

@ansibot ansibot removed the needs_revision label Jan 3, 2017

@jasperla

This comment has been minimized.

Contributor

jasperla commented Jan 3, 2017

Thanks @gundalow for your feedback I've implemented your requested changes.

@gundalow

This comment has been minimized.

Contributor

gundalow commented Jan 4, 2017

@jasperla Thanks for the updates. FYI, no need to force update, use separate commits once you've had your first review, that makes reviewing easier. We squash the commits when merging.

Can you please update CHANGELOG.md in the root of the Ansible Repo to add this to the new module list under 2.3

@jasperla

This comment has been minimized.

Contributor

jasperla commented Jan 4, 2017

@gundalow All ready!

@gundalow

shipit

cmd = IMGADM + ' update'
if uuid != '*':
cmd += ' {}'.format(uuid)

This comment has been minimized.

@abadger

abadger Jan 5, 2017

Member

Note: This syntax is python-2.7 or greater. Using .format() with positions is 2.6+:

cmd += ' {0}'.format(uuid)

It's more pythonic to create a single string rather than two. for instance:

cmd = '{0} {1}'.format(cmd, uuid)
# or
cmd = ' '.join((cmd, uuid))

If you need to be compatible with python less than 2.6, you need to avoid .format() altogether. So the .join method or percent formatting:

cmd = '%s %s' % (cmd, uuid)
uuid:
required: false
description:
- Image UUID. Can either be a full UUID or C(*) for all images.

This comment has been minimized.

@abadger

abadger Jan 5, 2017

Member

If this requires a higher python version than 2.4 then there needs to be a requirements section that lists it (for instance):

    requirements:
        - python >= 2.6

This comment has been minimized.

@sivel

sivel Jan 5, 2017

Member

I don't see any imports that would dictate that this require python 2.6 or higher.

This comment has been minimized.

@jasperla

jasperla Jan 5, 2017

Contributor

The reason I went for that syntax is that python 2.7 the only version available on SmartOS in the global zone for the past 3 years. I can therefore safely set the requirement to python >= 2.7 even, no?

EDIT: I went for python >= 2.6.

def manage_sources(module, present):
force = module.params['force']
source = module.params['source']
type = module.params['type']

This comment has been minimized.

@abadger

abadger Jan 5, 2017

Member

Note (not a blocker): type overrides the builtn "type". It's better to use a different name even if it's type_.

This comment has been minimized.

@jasperla

jasperla Jan 5, 2017

Contributor

I'll rename it to imgtype.

pool = module.params['pool']
state = module.params['state']
if state == 'vacumed':

This comment has been minimized.

@abadger

abadger Jan 5, 2017

Member

note, the spelling of this word is: vacuumed. Probably should change it everywhere.

This comment has been minimized.

@jasperla

jasperla Jan 5, 2017

Contributor

Good point, I'll adjust that too.

@abadger

This comment has been minimized.

Member

abadger commented Jan 5, 2017

A few things to fix. The most widespread is probably adjusting the file for the minimum python version.

As sivel pointed out, the rule is "minimum python version (down to python2.4) which the libraries the ansible module depends on require." I see that this is passing tests, anyway, though... I think the whitelist for the compile test blankets the entire cloud directory and that's why. We probably should make that more targetted when we have a chance to go through the modules there.

@abadger abadger merged commit 7793424 into ansible:devel Jan 5, 2017

1 check was pending

Shippable Run 8656 status is PROCESSING.
Details
@abadger

This comment has been minimized.

Member

abadger commented Jan 5, 2017

Cool. Merged to the devel branch. Thanks @jasperla !

@jasperla

This comment has been minimized.

Contributor

jasperla commented Jan 5, 2017

Thanks a lot @gundalow and @abadger for your feedback!

@jasperla jasperla deleted the jasperla:imgadm branch Jan 5, 2017

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