Skip to content
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 Dockerfile #70

Merged
merged 3 commits into from
Dec 31, 2017
Merged

add Dockerfile #70

merged 3 commits into from
Dec 31, 2017

Conversation

JordanSussman
Copy link
Contributor

@JordanSussman JordanSussman commented Dec 28, 2017

changes

I've added a fairly basic Dockerfile that is based on python:alpine3.6

usage

  1. docker build .
  2. docker run <image> <notifiers command args>

Ideally the image would be uploaded to docker hub via a automated build, but travis ci would also work.

I don't see the workflow to publish notifiers to pypi, so if it makes more sense to build from source I'm happy to update the PR with:

FROM python:alpine3.6

ADD . /notifiers
WORKDIR /notifiers

RUN pip install --upgrade pip setuptools
RUN pip install -e .
RUN pip install -r requirements.txt

ENTRYPOINT ["notifiers"]

Thanks!
Jordan

@liiight
Copy link
Owner

liiight commented Dec 28, 2017

Thanks for this! I have a docker hub account although my usage of docker is so minimal it didn't even occur to me to release there as well.
Could you point me in the right direction on how to correctly do that?

Also, the tests are failing since prs cannot access secure env vars which are used for the online tests. I thoguht I figured out that issue but obviously it doesn't work. I may use you as a test subject to test the workaround since I can't do it myself.

@liiight
Copy link
Owner

liiight commented Dec 28, 2017

ok, looked into it. Please push the changes you mentioned in the comment to build from source and ill setup the automated builds.

Also i added some debugging to the logs to try and figure out what i did wrong with skipping these online tests from prs

@JordanSussman
Copy link
Contributor Author

Thanks for the quick response. I've updated the Dockerfile to build from source.

Happy to help test the Travis changes.

@liiight
Copy link
Owner

liiight commented Dec 29, 2017

i pushed a fix to the test issue, please sync

@codecov
Copy link

codecov bot commented Dec 30, 2017

Codecov Report

Merging #70 into develop will decrease coverage by 13.97%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           develop      #70       +/-   ##
============================================
- Coverage       88%   74.03%   -13.98%     
============================================
  Files           39       39               
  Lines         1317     1317               
============================================
- Hits          1159      975      -184     
- Misses         158      342      +184
Impacted Files Coverage Δ
tests/providers/test_slack.py 50% <0%> (-50%) ⬇️
notifiers/providers/slack.py 37.14% <0%> (-48.58%) ⬇️
tests/providers/test_pushover.py 51.51% <0%> (-48.49%) ⬇️
tests/providers/test_gitter.py 57.89% <0%> (-42.11%) ⬇️
notifiers/providers/gmail.py 58.33% <0%> (-41.67%) ⬇️
notifiers/providers/zulip.py 42.5% <0%> (-40%) ⬇️
notifiers/providers/pushover.py 54.54% <0%> (-39.4%) ⬇️
tests/providers/test_telegram.py 61.11% <0%> (-38.89%) ⬇️
notifiers/providers/simplepush.py 45.83% <0%> (-33.34%) ⬇️
tests/providers/test_zulip.py 68% <0%> (-32%) ⬇️
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 53859b6...b4d3aa9. Read the comment docs.

@JordanSussman
Copy link
Contributor Author

JordanSussman commented Dec 30, 2017

@liiight the travis tests look good. I'm not really sure why codecov is complaining about lower coverage though.

Would you like me to squash the commits?

@liiight liiight merged commit 950d9ef into liiight:develop Dec 31, 2017
@liiight
Copy link
Owner

liiight commented Dec 31, 2017

codecov complains because now a lot of the test are skipped which appears to drop the coverage, don't worry about it.

Also, I squashed on merge so no need for any more action on you end. Thanks for this! I'll update here as well when i setup the automated docker build,

@liiight
Copy link
Owner

liiight commented Dec 31, 2017

done: https://hub.docker.com/r/liiight/notifiers/

i'm pushing 0.6.2 so it'll populate master branch

@JordanSussman JordanSussman deleted the docker branch December 31, 2017 20:59
@JordanSussman
Copy link
Contributor Author

Thanks!

fyi - Looks like the auto formatted readme on https://hub.docker.com/r/liiight/notifiers/ isn't rendering the badges. I'm not very familiar with RST, but the following Markdown will work correctly on Dockerhub.

[![Travis CI](https://img.shields.io/travis/liiight/notifiers/master.svg?style=flat-square)](https://travis-ci.org/liiight/notifiers)

[![Codecov](https://img.shields.io/codecov/c/github/liiight/notifiers/master.svg?style=flat-square)](https://travis-ci.org/liiight/notifiers)

[![Gitter](https://img.shields.io/gitter/room/nwjs/nw.js.svg?style=flat-square)](https://gitter.im/notifiers/notifiers)

[![PyPi version](https://img.shields.io/pypi/v/notifiers.svg?style=flat-square)](https://pypi.python.org/pypi/notifiers)

[![Supported Python versions](https://img.shields.io/pypi/pyversions/notifiers.svg?style=flat-square)](https://pypi.org/project/notifiers)

[![License](https://img.shields.io/pypi/l/notifiers.svg?style=flat-square)](https://choosealicense.com/licenses)

[![Status](https://img.shields.io/pypi/status/notifiers.svg?style=flat-square)](https://pypi.python.org/pypi/notifiers)

[![Docker build](https://img.shields.io/docker/build/liiight/notifiers.svg?style=flat-square)](https://hub.docker.com/r/liiight/notifiers/)

@liiight
Copy link
Owner

liiight commented Dec 31, 2017

The problem with that is that pypi requires rst...
I'll probably end up having both, generating one on the fly from the other

@JordanSussman
Copy link
Contributor Author

Based on docker/hub-feedback#621 it looks like Docker isn't in any hurry to add rst support 😞. If you wanted to get it fixed in the short term you could manually modify the full description on https://hub.docker.com/r/liiight/notifiers/. However, it isn't a big deal, so eventually having a markdown readme would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants