This repository has been archived by the owner. It is now read-only.

Tweak and extend the pkgin module #2010

Merged
merged 1 commit into from Apr 13, 2016

Conversation

Projects
None yet
5 participants
@jasperla
Copy link
Contributor

jasperla commented Apr 12, 2016

ISSUE TYPE
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME

pkgin

ANSIBLE VERSION
ansible 2.0.1.0
  config file =
  configured module search path = Default w/o overrides
SUMMARY

This PR combines several changes:

  • make path to pkgin a global and stop passing it around; as it's not going to change while ansible is running we can determine it once and just re-use it instead of passing it to various function calls.
  • add support for several new options:
    • upgrade
    • full_upgrade
    • force
    • clean
      These are equivalent to the pkgin options upgrade, full-upgrade, -F and clean respectively. For example it allows for updating all the currently installed packages in a zone with:
    - name: upgrade all packages
      pkgin: full_upgrade=yes update_cache=yes
  • allow for update_cache to be run in the same task as upgrading/installing packages instead of needing a separate task for that. Previously it was required to use:
    - name: update cache
      pkgin:  update_cache=yes
    - name: install tmux
      pkgin: name=tmux state=present

Now the following works as expected:

    - name: install tmux
      pkgin:  update_cache=yes name=tmux state=present

Running the following tasks before failed:

    - name: upgrade all packages
      pkgin: full_upgrade=yes update_cache=yes

    - name: install base packages
      pkgin: name={{ item }} state=present update_cache=yes
      with_items:
        - go
        - tmux
        - wget
TASK [upgrade all packages] ****************************************************
fatal: [dsapid]: FAILED! => {"changed": false, "failed": true, "msg": "unsupported parameter for module: full_upgrade"}

&c

Now it works as expected:

TASK [upgrade all packages] ****************************************************
ok: [dsapid] => {"changed": false, "msg": "nothing left to upgrade"}

TASK [install base packages] ***************************************************
ok: [dsapid] => (item=go) => {"changed": false, "item": "go", "msg": "package(s) already present"}
ok: [dsapid] => (item=tmux) => {"changed": false, "item": "tmux", "msg": "package(s) already present"}
ok: [dsapid] => (item=wget) => {"changed": false, "item": "wget", "msg": "package(s) already present"}
@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Apr 12, 2016

@troy2914 Could you please have a look at this?

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 12, 2016

Thanks @jasperla for this PR. This PR requires revisions, either because it fails to build or by reviewer request. Please make the suggested revisions. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

[This message brought to you by your friendly Ansibull-bot.]

@sivel

This comment has been minimized.

Copy link
Member

sivel commented Apr 12, 2016

This appears to be failing Python 2.4 syntax checks due to:

Compiling ./packaging/os/pkgin.py ...
  File "./packaging/os/pkgin.py", line 214
    force = "-F" if module.params["force"] else ""
                  ^
SyntaxError: invalid syntax

The recommendation here is to expand this out into a full if/else statement instead of using ternary syntax.

needs_revision

@jasperla jasperla force-pushed the jasperla:pkgin branch from 02ad8ae to 889604f Apr 12, 2016

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Apr 12, 2016

@sivel thanks, it's all green now :)

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Apr 12, 2016

I am not clear on why required_one_of (line 329) includes force, force cannot be used by itself can it?

@jasperla jasperla force-pushed the jasperla:pkgin branch from 889604f to 77eb174 Apr 12, 2016

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Apr 12, 2016

@troy2914 that is correct and it was included by accident.

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Apr 12, 2016

shipit

@gregdek gregdek removed the needs_revision label Apr 12, 2016

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 12, 2016

Thanks again to @jasperla for this PR, and thanks @L2G, @szinck, @troy2914 for reviewing. Marking for inclusion.

[This message brought to you by your friendly Ansibull-bot.]

format_pkgin_command(module, cmd))

if rc == 0:
if re.search('^nothing to do.\n$', out):

This comment has been minimized.

@resmo

resmo Apr 13, 2016

Member

we must make sure the LANG is set correctly if we screen scrape commands stdouts.

The safe way is to add

module.run_command_environ_update = dict(LANG='C', LC_ALL='C', LC_MESSAGES='C', LC_CTYPE='C')

somewhere in the main() function so it will be applied in every command.

This comment has been minimized.

@jasperla

jasperla Apr 13, 2016

Contributor

Thanks, I've updated this PR accordingly.

Looking at other modules that do screen scraping, it seems they'll need the same medicine. I'll submit a separate PR for those.

Tweak and extend the pkgin module:
- make path to pkgin a global and stop passing it around; it's not going
  to change while ansible is running
- add support for several new options:
  * upgrade
  * full_upgrade
  * force
  * clean
- allow for update_cache to be run in the same task as upgrading/installing
  packages instead of needing a separate task for that

@jasperla jasperla force-pushed the jasperla:pkgin branch from 77eb174 to 8a1f72d Apr 13, 2016

@resmo resmo merged commit 85c1440 into ansible:devel Apr 13, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@troy2914

This comment has been minimized.

Copy link

troy2914 commented Apr 13, 2016

@jasperla I think you need to open a new request for the LANG cleanup here.

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Apr 13, 2016

@troy2914, @resmo already merged it with the LANG cleanup.

@jasperla jasperla deleted the jasperla:pkgin branch Apr 13, 2016

teochenglim pushed a commit to teochenglim/ansible-modules-extras that referenced this pull request Jul 27, 2016

Merge pull request ansible#2010 from lberruti/global_user_crontab_env…
…_variables_rebase

cron module: add enviroment variables management
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.