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

support for 'update_cache' in pkgin module #1827

Merged
merged 1 commit into from Apr 10, 2016

Conversation

Projects
None yet
4 participants
@jasperla
Copy link
Contributor

jasperla commented Mar 10, 2016

Issue Type:
  • Feature Pull Request
Plugin Name:

pkgin

Ansible Version:
ansible 2.0.1.0
  config file =
  configured module search path = Default w/o overrides
Summary:

pkgin uses a locally cached copy of a packages database to lookup package names, and so forth. Currently the pkgin module has no way to update this database. Common practise in other modules is through a update_cache option, which is now implemented for pkgin too.

Example output:
TASK [update cache] ************************************************************
changed: [host] => {"changed": true, "msg": "updated repository database"}

cc'ing @L2G for extra review as he most recently worked on this module

@jasperla jasperla force-pushed the jasperla:pkgin_update_cache branch 2 times, most recently from 3939382 to ec3c807 Mar 10, 2016



def main():
module = AnsibleModule(
argument_spec = dict(
state = dict(default="present", choices=["present","absent"]),
name = dict(aliases=["pkg"], required=True)),
name = dict(aliases=["pkg"]),
update_cache = dict(default='no', choices=BOOLEANS, type='bool')),

This comment has been minimized.

@resmo

resmo Mar 10, 2016

Member

remove choices=BOOLEANS, its deprecated. type=bool only should be used.

This comment has been minimized.

@jasperla

jasperla Mar 10, 2016

Contributor

ack.

@resmo

This comment has been minimized.

Copy link
Member

resmo commented Mar 10, 2016

Please update the doc for arg name: remove required: true and add default: null

@jasperla jasperla force-pushed the jasperla:pkgin_update_cache branch from ec3c807 to 9214975 Mar 10, 2016

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Mar 10, 2016

@resmo I've updated this PR with your feedback, thanks.

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Mar 11, 2016

Thanks @jasperla. @L2G @szinck please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Mar 11, 2016

@jasperla per the guidelines, required: should have been changed to false, vs removing it.

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Mar 11, 2016

@troy2914 ok, so then I'll re-add required back and keep the default: null ?

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Mar 11, 2016

yep, from the guidelines:
"required should always be present, be it true or false
If required is false you need to document default, even if the default is ‘null’ (which is the default if no parameter is supplied). Make sure default parameter in docs matches default parameter in code."

@jasperla jasperla force-pushed the jasperla:pkgin_update_cache branch from 9214975 to 61de618 Mar 12, 2016

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Mar 12, 2016

@troy2914 thanks, it's updated now. I've got a whole bunch of other features for this provider ready when this PR has been merged (upgrade, full_upgrade, clean and some refactoring).

@resmo resmo added needs_rebase and removed community_review labels Mar 25, 2016

@resmo

This comment has been minimized.

Copy link
Member

resmo commented Mar 25, 2016

Thanks @jasperla for this PR. Unfortunately, it is not mergeable in its current state due to merge conflicts. Please rebase your PR. When you are done, please comment with text 'ready_for_review' and we will put this PR back into review.

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Mar 29, 2016

Ping?

@resmo

This comment has been minimized.

Copy link
Member

resmo commented Mar 29, 2016

@jasperla yes? did you see my comment? It is still not mergeable. Let me know if you need assistance.

@jasperla jasperla force-pushed the jasperla:pkgin_update_cache branch from 61de618 to 9959d97 Mar 29, 2016

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Mar 29, 2016

Silly me, I completely missed your comment.

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Mar 29, 2016

ready_for_review

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Mar 29, 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.]

@gregdek gregdek added needs_revision and removed needs_rebase labels Mar 29, 2016

@jasperla jasperla force-pushed the jasperla:pkgin_update_cache branch from 9959d97 to 88a3734 Mar 29, 2016

@jasperla jasperla force-pushed the jasperla:pkgin_update_cache branch from 88a3734 to 3e31c24 Mar 29, 2016

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Mar 30, 2016

ready_for_review

@gregdek gregdek removed the needs_revision label Mar 30, 2016

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Mar 30, 2016

Thanks @jasperla. To the current maintainers, @L2G, @szinck please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

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

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Apr 6, 2016

@gregdek what's the policy if neither maintainers respond in a timely manner? I'd love to move forward with this module.

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 6, 2016

Typically, we give maintainers a couple of weeks to respond before we politely remind them, and a couple of weeks after that we start to look for additional reviewers.

In this case, I'm happy to hurry it along a bit. @troy2914, I see that you gave some useful reviews; thanks for them. Would you like to consider becoming an official maintainer for this module? It would basically consist of doing the same thing you already did for this module. :)

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Apr 6, 2016

Is there some documentation on expectations for an official maintainer for this module? (beyond the module-checklist) I've read the nice flow chart in REVIEWERS.md too.

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 6, 2016

Yes:

http://docs.ansible.com/ansible/developing_modules.html#module-checklist

Each time there's a PR to a module you maintain, you should get a ping from the bot asking for your review and reminding you of how to do it (see comment 3 above).

What do you say? :)

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Apr 6, 2016

Okay, I accept.

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 6, 2016

OK, you're in. :)

Next bot run (6 hours-ish) and it should be official, and your next "shipit" will promote this PR for potential inclusion.

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 6, 2016

And thank you!

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Apr 6, 2016

Note I am not sure the version_added: "2.1" is correct, given the current version of the development is listed in VERSION as "2.0.0-0.5.beta3"?

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 6, 2016

Thanks @jasperla. To the current maintainers, @L2G, @szinck, @troy2914 please review according to guidelines (http://docs.ansible.com/ansible/developing_modules.html#module-checklist) and comment with text 'shipit' or 'needs_revision' as appropriate.

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

@troy2914

This comment has been minimized.

Copy link

troy2914 commented Apr 6, 2016

shipit

@gregdek

This comment has been minimized.

Copy link
Contributor

gregdek commented Apr 7, 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.]

@gregdek gregdek added the shipit label Apr 7, 2016

@jasperla

This comment has been minimized.

Copy link
Contributor

jasperla commented Apr 7, 2016

@gregdek @troy2914 thanks guys, I really appreciate your efforts!

@resmo resmo merged commit 542a96f into ansible:devel Apr 10, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.