-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
Thanks for submitting this module to Ansible Extras. Apologies that it’s taken a while to get your module reviewed. To help facilitate reviews, we’ve broadened the number of people who can approve modules for inclusion into the Extras repository. The list of official reviewers can be found here: https://github.com/ansible/ansible-modules-extras/blob/devel/REVIEWERS.md Our new policy is that if a new module is reviewed and approved by at least two official module reviewers, the module will be approved for inclusion. We will be asking the community of reviewers to take a look at these modules on a regular basis. To ensure that your module has the best chance of being approved, please double-check that you adhere to the Ansible module guidelines: http://docs.ansible.com/developing_modules.html#module-checklist |
I went through the checklist and it seems like everything is in order. |
Hi monitoring module owners -- @bpennypacker @hkariti @ccollicutt @arturaz @skornehl @bwhaley @abulimov -- anyone interested in helping with another sensu module review? (See #203 for the first one) |
6ce4192
to
6faf04c
Compare
I applied the comments from #203 to this PR as well. |
f7418e6
to
f33eded
Compare
OK. I fixed the errors. Man, python 2.4 compat is a PITA |
I added some 2.4 compat, but I can see you already merged. You can apply this instead if you like: |
Thanks @andsens -- still looking for reviewers for the module itself. |
Any chance this will be merged soon? It goes well with the |
Hi @andsens -- comments below are re: the new process for PRs in extras. We will be evaluating all new module PRs according to this process, effective immediately. If you have any sensu + ansible loving friends, they are welcome to follow the guidelines below and help get this in :) Thanks for submitting this new module to Ansible Extras! This module is now in community review, a process that is open to all Ansible users. In order for this module to be approved, it must gain the following votes: “works_for_me”: If you have tested the module thoroughly, including testing of all of the module’s options, and if the module works for you, please add “works_for_me” in the comments. “passes_guidelines”: If you have gone through the module guidelines and the module meets all of the requirements, please add “passes_guidelines” in the comments. Guidelines are available here: http://docs.ansible.com/developing_modules.html#module-checklist “needs_revision”: If the module fails to work for you, or if it doesn’t meet guidelines, please add “needs_revision” in the comments with details about what needs to be fixed. When a module has both “works_for_me” and “passes_guidelines” tags, we will promote the module for inclusion in Ansible Extras. At this point, you will be expected to maintain the module by fixing bugs and evaluating pull requests in a timely manner. Thanks again for submitting your Ansible module! |
Any chance this can get merged soon? works_for_me |
Thanks @andsens 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.] |
ready_for_review
|
Yep, unrelated breakage. Should resolve itself when the issue is fixed in the base branch, at which point this should flip back to community_review. @andsens give it a couple of days and we'll see. If not, ping me in the comments. |
Thanks @andsens 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.] |
module.exit_json(path=path, name=name, changed=changed, msg='OK', reasons=reasons) | ||
|
||
from ansible.module_utils.basic import * | ||
main() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs to be as follows so debuggers can work with ziploader:
if __name__ == '__main__':
main()
@gundalow are these the only changes you see being needed? If so, should we consider a merge and fix? |
I'm happy with a merge-and-fix |
@bcoca will address my comments once this has been merged shipit |
@gregdek, no probs. I'm just glad to see it merged ;-) |
Thanks @andsens 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.] |
@bcoca I believe you offered to merge and fix this, for reference the two items that need tweaking are:
|
@andsens A friendly reminder: this pull request has been marked as needing your action. If you still believe that this PR applies, and you intend to address the issues with this PR, just let us know in the PR itself and we will keep it open pending your changes. When you do address the issues, please respond with ready_for_review in your comment, so that we can notify the maintainer. [This message brought to you by your friendly Ansibull-bot.] |
dbbf2b3
to
5953864
Compare
ready_for_review Please merge this one before 2.2 gets released :-) |
Thanks @andsens for this new module. When this module receives 'shipit' comments from two community members and any 'needs_revision' comments have been resolved, we will mark for inclusion. [This message brought to you by your friendly Ansibull-bot.] |
Shipit @bcoca I believe you offers to merge this in a previous new modules meeting, could you please do the necessary. Thanks |
shipit |
try: | ||
open(path, 'w').write(json.dumps(config, indent=2) + '\n') | ||
except IOError, e: | ||
module.fail_json(msg=str(e)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we normally want to return a more 'friendly' error message to the user, but leaving that for a follow up (same issue in a couple of places):
'We could not write to the %s file: %s' % (config, str(e))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Syntax at 132 is breaking jobs
This PR goes well with PR #203.
The module
sensu_subscription
manages sensu subscriptions of the sensu client running on a machine.