search

psyinfra / prometheus-eaton-ups-exporter

A Prometheus exporter for (some) Eaton UPSs
ISC License
11 stars 5 forks source link

Remove `urllib3` dependency for `requests` functionality #28

Closed TobiasKadelka closed 3 months ago

TobiasKadelka commented 5 months ago

Removing the unnecessary dependency should be addressed in a future PR.

Out of scope, but this dependency is used in one place, and simply to disable SSL verification.

requests supports this feature directly. https://requests.readthedocs.io/en/latest/user/advanced/#ssl-cert-verification

_Originally posted by @aqw in https://github.com/psyinfra/prometheus-eaton-ups-exporter/pull/27#discussion_r1542593793_

TobiasKadelka commented 5 months ago

I think the comment above is partly wrong.

The eaton ups exporter already uses requests to disable the SSL verification with: self.session.verify = not insecure # ignore self signed certificate, but this is not what it uses urllib3 for.

When using requests session.verify = False this means SSL verification is ignored, which the people of requests discussed, and they decided that that makes a warning about this behavior necessary.

It is possible to use urllib3.disable_warnings() to disable these warnings -> and this is what is what the eaton ups exporter is doing and what urllib3 is used for.

TobiasKadelka commented 5 months ago

Discussion about this in requests: https://github.com/psf/requests/issues/2214

To remove the dependency we could just allow the warning to happen.

aqw commented 5 months ago

Ahh, thanks for sharing that. Indeed that's different than I thought.

I would prefer a solution more in the line of one of these two:

They both solve this problem different ways, but in common, they import from the requests namespace, which makes the intent and relation more obvious.

nhjjr commented 5 months ago

urllib3 is a dependency for requests, so it's going to be part of the package environment anyway, but I vastly prefer the solution @aqw suggested. Note that this is a remnant directly copied from the Raritan PDU Exporter back when it used requests and multi-threading instead of aiohttp and asynchronous operations.

Perhaps it is worth considering doing the same for this exporter, if the time investment is deemed worthwhile.