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

Use gunicorn instead of the Werkzeug development server #132

Merged
merged 3 commits into from
Apr 26, 2023

Conversation

crabhi
Copy link
Contributor

@crabhi crabhi commented Mar 1, 2023

Werkzeug itself doesn't recommend being used in production.

$ pve_exporter                                                                                                 
WARNING: This is a development server. Do not use it in a production deployment. Use a production WSGI server instead.
 * Running on http://:9221
Press CTRL+C to quit

Using Gunicorn brings in e.g. HTTPS support with low effort.

Resolves #130.

@crabhi crabhi mentioned this pull request Mar 7, 2023
'bind': f'{params.address}:{params.port}',
'threads': 2,
'keyfile': params.__dict__['server.keyfile'],
'certfile': params.__dict__['server.certfile'],
Copy link
Member

Choose a reason for hiding this comment

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

There is a way to avoid using params.__dict__ here: Add a dest argument in the parser.add_argument call above in order to specify the key name. I suggest the following key names:

parser.add_argument('--server.keyfile', dest='server_keyfile'
                    help='SSL key for server')
parser.add_argument('--server.certfile', dest='server_certfile'
                    help='SSL certificate for server')

This way the values can be accessed using the property syntax.

    'keyfile': params.server_keyfile,
    'certfile': params.server_certfile,

Comment on lines +122 to +129
def load_config(self):
config = {key: value for key, value in self.options.items()
if key in self.cfg.settings and value is not None}
for key, value in config.items():
self.cfg.set(key.lower(), value)
Copy link
Member

Choose a reason for hiding this comment

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

This is hardly readable. However, it follows the upstream example and I honestly have no better idea on how to translate the options dictionary into the gunicorn cfg format. Hence, I'm okay with it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. At least I added a comment pointing out the source.

@znerol
Copy link
Member

znerol commented Mar 17, 2023

Also, please add py3-gunicorn to the list of packages in Dockerfile.

@crabhi crabhi requested a review from znerol March 27, 2023 15:05
@znerol
Copy link
Member

znerol commented Apr 7, 2023

Cool thanks, meanwhile I fixed my own linting errors in the develop branch. Would you mind rebasing this PR to develop? Github UI doesn't allow me to do this myself.

crabhi added 3 commits April 25, 2023 16:54
Werkzeug itself doesn't recommend being used in production. Using Gunicorn
brings in e.g. HTTPS support with low effort.
@znerol znerol changed the base branch from main to develop April 26, 2023 11:23
@znerol
Copy link
Member

znerol commented Apr 26, 2023

Thanks a lot. I've tested this PR locally with and without HTTPS. Also tested the docker build, everything looks great!

@znerol znerol merged commit d88f0af into prometheus-pve:develop Apr 26, 2023
@crabhi crabhi deleted the gunicorn-ssl branch April 27, 2023 15:21
@znerol znerol mentioned this pull request Aug 2, 2023
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.

https support for exporter
2 participants