python3 support for letsencrypt module (fixes #30690) #32734
Conversation
* decorate fetch_url to upsert lowercase representation of the header names returned in info-dict * initialize result to a dict in some methods to prevent 'NoneType is not iterable' TypeError * use dict.get() to retrieve values from info dict to prevent KeyError * convert to/from text/bytes using _text methods for PY3 support
|
The test The test The test |
ansibot
added
affects_2.5
samdoran
removed
the
needs_triage
ansibot
added
community_review
ansibot
added
the
stale_ci
| return response, info | ||
| return func_wrapper | ||
|
|
||
| fetch_url = _upsert_lowercase_headers(fetch_url) |
There was a problem hiding this comment.
I would just make this a plain function that calls fetch_url and then lowercases the keys instead of making it a decorator. I'm a little hesitant about overriding the name (I'd use a different name than fetch_url) but I can see you do that so you don't have to change the name everywhere. Since I'm not the module author, I won't block it over that but I think it's better style to use a new name.
There was a problem hiding this comment.
The reason I've used a decorator was indeed to prevent having to rename all fetch_url occurrences. And since I'm not altering the values but merely ensuring the lowercase representation is included, I figured a decorator would be easier.
Maybe this should be fixed in the fetch_url module utility ? Otherwise every module that uses this function AND wants to support py3 needs to create a wrapper or create several conditionals to capture header case variations ?
| @@ -355,7 +370,7 @@ def _parse_account_key(self, key): | |||
|
|
|||
| pub_hex, pub_exp = re.search( | |||
| r"modulus:\n\s+00:([a-f0-9\:\s]+?)\npublicExponent: ([0-9]+)", | |||
| out.decode('utf8'), re.MULTILINE | re.DOTALL).groups() | |||
| to_text(out), re.MULTILINE | re.DOTALL).groups() | |||
There was a problem hiding this comment.
Unless simply constructing a message for the user, use to_text, to_bytes, etc with errors='surrogate_or_strict' like this:
to_text(out, errors='surrogate_or_strict'), re.MULTILINE | re.DOTALL).groups()There was a problem hiding this comment.
Ok, have added the error handler argument.
ansibot
removed
the
stale_ci
|
We can't change it there without breaking backwards compatibility and it's
not obviously a general fix (case sensitivity is part of the headers even
if it would be inside of an application to make use of it.
A decorate is inappropriate here because you are not decorations the
fetch_url function. A normal wrapper function is the way to go
|
|
Autocorrect mangled my last message: We can't change fetch_url's
headers to be case insensitive without breaking backwards
compatibility of its API. It's not obviously a general fix because
headers are case sensitive. A web application could be depending upon
the case being correct even if it was bad practice (ie, doing
something different with HEADER vs header).
The normal function wrapper advice still stands. Perhaps you missed
it in my original comment, though: you can use the name fetch_url and
I'll still merge this (maintainer can change it later if they want to
fix it in a different way), it's just making it a decorator that
doesn't make sense here:
``` python
def _normalize_to_lowercase(*args, **kwargs):
"""Add lowercase representations of the header names as dict keys"""
response, info = fetch_url(*args, **kwargs)
info.update(dict(header.lower(), value) for (header, value) in
info.items()))
return response, info
fetch_url = _normalize_to_lowercase
```
…On Mon, Dec 4, 2017 at 11:08 PM, Toshio Kuratomi ***@***.***> wrote:
We can't change it there without breaking backwards compatibility and it's
not obviously a general fix (case sensitivity is part of the headers even if
it would be inside of an application to make use of it.
A decorate is inappropriate here because you are not decorations the
fetch_url function. A normal wrapper function is the way to go
|
|
@abadger have updated this to be a function wrapper instead. I've kept the name fetch_url for now - wouldn’t want to mangle too much with the rest of the module without maintainer's involvement. |
* python3 support for letsencrypt module (fixes #30690) * initialize result to a dict in some methods to prevent 'NoneType is not iterable' TypeError * use dict.get() to retrieve values from info dict to prevent KeyError * convert to/from text/bytes using _text methods for PY3 support (cherry picked from commit 3a63405)
|
Merged to devel and cherry-picked to the stable-2.4 branch for inclusion in 2.4.3 beta1 |
|
thanks @abadger for stepping in. |
ansibot
added
bug
decorate fetch_url to upsert lowercase representation of the header names returned in info-dict
initialize result to a dict in some methods to prevent 'NoneType is not iterable' TypeError
use dict.get() to retrieve values from info dict to prevent KeyError
convert to/from text/bytes using _text methods for PY3 support
SUMMARY
fixes #30690
ISSUE TYPE
COMPONENT NAME
lib/ansible/modules/web_infrastructure/letsencrypt.py
ANSIBLE VERSION