Add honeybadger_deployment module #32

Closed
wants to merge 1 commit into
from

Projects

None yet

7 participants

@stympy
Contributor
stympy commented Sep 30, 2014
@mpdehaan mpdehaan added the new_module label Dec 8, 2014
@gregdek
Contributor
gregdek commented Jun 16, 2015

Thanks for submitting your module to Ansible Extras. Our policy is that each new module needs to be reviewed and approved by at least two official module reviewers, the list of whom can be found here: https://github.com/ansible/ansible-modules-extras/blob/devel/REVIEWERS.md

If you know someone on that list, please poke them and encourage them to review your module. :)

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

@gregdek gregdek commented on an outdated diff Aug 28, 2015
monitoring/honeybadger_deployment.py
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation, either version 3 of the License, or
+# (at your option) any later version.
+#
+# Ansible is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with Ansible. If not, see <http://www.gnu.org/licenses/>.
+
+DOCUMENTATION = '''
+---
+module: honeybadger_deployment
+author: Benjamin Curtis
@gregdek
gregdek Aug 28, 2015 Contributor

"Author should be set, name and github id at least"
...i.e. "Benjamin Curtis (@stympy)"

@gregdek gregdek commented on the diff Aug 28, 2015
monitoring/honeybadger_deployment.py
+short_description: Notify Honeybadger.io about app deployments
+description:
+ - Notify Honeybadger.io about app deployments (see http://docs.honeybadger.io/article/188-deployment-tracking)
+options:
+ token:
+ description:
+ - API token.
+ required: true
+ environment:
+ description:
+ - The environment name, typically 'production', 'staging', etc.
+ required: true
+ user:
+ description:
+ - The username of the person doing the deployment
+ required: false
@gregdek
gregdek Aug 28, 2015 Contributor

If required is false you need to document default, even if its ‘None’, as in this case

@gregdek gregdek commented on an outdated diff Aug 28, 2015
monitoring/honeybadger_deployment.py
+ token:
+ description:
+ - API token.
+ required: true
+ environment:
+ description:
+ - The environment name, typically 'production', 'staging', etc.
+ required: true
+ user:
+ description:
+ - The username of the person doing the deployment
+ required: false
+ repo:
+ description:
+ - URL of the project repository
+ required: false
@gregdek
gregdek Aug 28, 2015 Contributor

If required is false you need to document default, even if its ‘None’, as in this case

@gregdek gregdek commented on an outdated diff Aug 28, 2015
monitoring/honeybadger_deployment.py
+ environment:
+ description:
+ - The environment name, typically 'production', 'staging', etc.
+ required: true
+ user:
+ description:
+ - The username of the person doing the deployment
+ required: false
+ repo:
+ description:
+ - URL of the project repository
+ required: false
+ revision:
+ description:
+ - A hash, number, tag, or other identifier showing what revision was deployed
+ required: false
@gregdek
gregdek Aug 28, 2015 Contributor

If required is false you need to document default, even if its ‘None’, as in this case

@gregdek gregdek commented on the diff Aug 28, 2015
monitoring/honeybadger_deployment.py
+ on personally controlled sites using self-signed certificates.
+ required: false
+ default: 'yes'
+ choices: ['yes', 'no']
+
+# informational: requirements for nodes
+requirements: [ urllib, urllib2 ]
+'''
+
+EXAMPLES = '''
+- honeybadger_deployment: token=AAAAAA
+ environment='staging'
+ user='ansible'
+ revision=b6826b8
+ repo=git@github.com:user/repo.git
+'''
@gregdek
gregdek Aug 28, 2015 Contributor

Add version_added parameter to current devel version: version_added: "2.0"

@gregdek gregdek commented on an outdated diff Aug 28, 2015
monitoring/honeybadger_deployment.py
+- honeybadger_deployment: token=AAAAAA
+ environment='staging'
+ user='ansible'
+ revision=b6826b8
+ repo=git@github.com:user/repo.git
+'''
+
+# ===========================================
+# Module execution.
+#
+
+def main():
+
+ module = AnsibleModule(
+ argument_spec=dict(
+ token=dict(required=True),
@gregdek
gregdek Aug 28, 2015 Contributor

For password / secret arguments no_log=True should be set (API tokens probably count here)

@gregdek gregdek commented on an outdated diff Aug 28, 2015
monitoring/honeybadger_deployment.py
+ try:
+ data = urllib.urlencode(params)
+ response, info = fetch_url(module, url, data=data)
+ except Exception, e:
+ module.fail_json(msg='Unable to notify Honeybadger: %s' % e)
+ else:
+ if info['status'] == 200:
+ module.exit_json(changed=True)
+ else:
+ module.fail_json(msg="HTTP result code: %d connecting to %s" % (info['status'], url))
+
+# import module snippets
+from ansible.module_utils.basic import *
+from ansible.module_utils.urls import *
+
+main()
@gregdek
gregdek Aug 28, 2015 Contributor

Call your main() from a conditional so that it would be possible to test them in the future example:

if __name__ == '__main__':
    main()
@stympy
Contributor
stympy commented Aug 28, 2015

Thanks for the review! I don't currently have the time to implement these suggestions, so I'm just going to close this.

@stympy stympy closed this Aug 28, 2015
@gregdek
Contributor
gregdek commented Aug 29, 2015

Thanks @stympy. If you ever want to resubmit, just let us know -- you're most of the way there!

@stympy
Contributor
stympy commented Aug 29, 2015

Ok, challenge accepted -- reopening :)

@stympy stympy reopened this Aug 29, 2015
@stympy
Contributor
stympy commented Aug 29, 2015

@gregdek I think I got everything you wanted in there

@robynbergeron
Contributor

Hi @stympy :) Thanks for implementing @gregdek's suggestions. I'm moving this back into community_review so it can move forward.

@robynbergeron
Contributor

Hi @stympy -- updating with new PR process info. We will be evaluating all new module PRs according to this process, effective immediately.

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!

@ryansb
Member
ryansb commented Sep 30, 2015

👍 passes_guidelines but I haven't been able to test it locally.

@robynbergeron
Contributor

@ryansb Thanks for the passes_guidelines thumbs up! :) (Yay, new process in action and working!)

Let us know if you are able to test it -- and of course, anyone else is still able to test this as well to move it towards inclusion.

@stympy
Contributor
stympy commented Oct 28, 2015

@robynbergeron I have been able to test this locally (again, just now), and it works as expected

@gregdek gregdek removed the P4 label Jan 26, 2016
@stympy
Contributor
stympy commented Mar 30, 2016

Can anyone comment on when this will be merged?

@gregdek
Contributor
gregdek commented Mar 31, 2016

@stympy it's failing travis.

Please make the suggested revisions, and when you're done, please add a comment with the text "ready_for_review" and we will continue with the review process. Thanks!

@stympy stympy closed this Mar 31, 2016
@stympy stympy reopened this Mar 31, 2016
@jimi-c jimi-c removed the needs_revision label Mar 31, 2016
@stympy
Contributor
stympy commented Mar 31, 2016

It looks like the build failure is unrelated to the PR. @gregdek can you take a look?

@gregdek
Contributor
gregdek commented Mar 31, 2016

Thanks @stympy 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.]

@gregdek
Contributor
gregdek commented Apr 1, 2016

@stympy the build error is strange. Waiting to see if it's transient. If not, ping me again in a few days and we'll dig in further.

@gregdek
Contributor
gregdek commented Apr 1, 2016

@stympy I haven't seen this error anywhere else. I'm wondering if it has to do with the age of the PR.

Would you mind creating a new PR for this? I know it's been open basically forever, but I'm hoping that our new new module progress will help us get it closed in not an insanely unreasonable time (sigh)

@stympy stympy closed this Apr 1, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment