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

added conversion to int for PORT #57

Merged
merged 4 commits into from
Apr 9, 2018
Merged

added conversion to int for PORT #57

merged 4 commits into from
Apr 9, 2018

Conversation

a-zen
Copy link

@a-zen a-zen commented Mar 28, 2018

Description of the Change

Converted given port to int to avoid the following startup error:

2018-03-28 07:52:57,324 - prom2teams_app - INFO - prom2teams started on 0.0.0.0:8089 Traceback (most recent call last): File "/usr/local/bin/prom2teams", line 15, in <module> application.run(host=host, port=port) File "/usr/local/lib/python3.6/site-packages/flask/app.py", line 841, in run run_simple(host, port, self, **options) File "/usr/local/lib/python3.6/site-packages/werkzeug/serving.py", line 748, in run_simple raise TypeError('port must be an integer') TypeError: port must be an integer

Benefits

Fixes error when running the application (inside docker).

Possible Drawbacks

Applicable Issues

@jnogol
Copy link
Collaborator

jnogol commented Mar 28, 2018

Hi @a-zen,

Thanks for your collaboration and for using Prom2Teams. I've just tried in my local and didn't encounter this issue. Could you please tell us the scenario where you ran into it?

Regards,
Jose

@a-zen
Copy link
Author

a-zen commented Mar 28, 2018

@jnogol

Yeah sure maybe I did something wrong. I used your docker container from dockerhub version 2.0.1.

docker run idealista/prom2teams:2.0.1

This gives me the above error.

@jnogol
Copy link
Collaborator

jnogol commented Mar 28, 2018

Hi again,

Couple of things. First of all, you're right, with our container on the 2.0.1 it fails, but because v2.0.1 is buggy itself: we forgot to cast to int the port again. Thanks for noticing and contributing.

In any case, I'll better the documentation to use our Docker image: running just

docker run idealista/prom2teams:VERSION

won't work: the connector URL needs to be pass as a parameter and container port 8089 needs to be mapped into the host. So, the command would be something like:

docker run -it -d -e PROM2TEAMS_CONNECTOR="CONNECTOR_URL" -p 8089:8089 idealista/prom2teams:VERSION

I'll update our readme to clarify that.

One last thing: please add this change to our CHANGELOG.md.

Once again, thanks for contributing,

Jose

@a-zen
Copy link
Author

a-zen commented Mar 29, 2018

Hi @jnogol,

thank you for the clarification. Yeah I saw from the dockerfile what I need to provide, but I wanted to show the minimal case one needs to do the reproduce the error. A documentation regarding the dockerfile would be very nice.

I updated the changelog. Please have a look.

@@ -7,6 +7,7 @@ This project adheres to [Semantic Versioning](http://semver.org/) and [Keep a ch
[Full Changelog](https://github.com/idealista/prom2teams/compare/2.0.0...2.0.1)
### Fixed
- *[#53](https://github.com/idealista/prom2teams/issues/53) Fix prom2teams uwsgi bin startup* @jmonterrubio
- *[#57](https://github.com/idealista/prom2teams/pull/57) added conversion to int for PORT* @a-zen
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be under "Unreleased" :D

@twittyc
Copy link

twittyc commented Apr 6, 2018

Hey guys, I'm hitting this bug. Can we merge this pull request? :)

@jnogol jnogol merged commit fd9ed24 into idealista:develop Apr 9, 2018
@jnogol jnogol mentioned this pull request Apr 9, 2018
@jnogol
Copy link
Collaborator

jnogol commented Apr 9, 2018

Version 2.0.2 released including this fix. Thanks @a-zen for your collaboration.

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.

5 participants