fix untyped-defs on /cloudinit/config & /tests/config/ #5985
Conversation
There was a problem hiding this comment.
@MostafaTarek124eru welcome to cloud-init! I left two comments inline. Also, please sign the cla.
cloudinit/config/cc_ubuntu_pro.py
Outdated
| @@ -225,7 +226,10 @@ def configure_pro(token, enable=None): | |||
| # related. We can distinguish them by checking if `service` is non-null | |||
| # or null respectively. | |||
|
|
|||
| enable_errors: List[dict] = [] | |||
|
|
|||
| def handle_enable_errors(enable_resp: Dict[str, Any]): | |||
There was a problem hiding this comment.
Making this a separate function makes the comment above nonsensical - it was describing this code yet it is no longer co-located with the code.
There was a problem hiding this comment.
I've moved the comment under the new function, Please check it
cloudinit/config/cc_rsyslog.py
Outdated
| @@ -246,6 +246,7 @@ def __init__( | |||
| self.proto = proto | |||
|
|
|||
| self.addr = addr | |||
| self.port = int(port) if port else None | |||
There was a problem hiding this comment.
This logic looks functionally identical to the next four lines. Why is it necessary?
There was a problem hiding this comment.
Fixed it in the new commit
cloudinit/config/cc_ubuntu_pro.py
Outdated
| except json.JSONDecodeError as e: | ||
| raise RuntimeError( | ||
| f"Pro response was not json: {enable_stdout}" | ||
| ) from e | ||
|
|
||
|
|
||
| def handle_enable_errors(enable_resp: Dict[str, Any]): |
There was a problem hiding this comment.
This silences valid mypy warnings without solving the underlying issue
consider:
>>> import json
>>> json.loads("[]")
[]
But this just told mypy that the type will be Dict[str, Any].
There was a problem hiding this comment.
Please check the new commit. It should handle it now
| self.port = int(port) | ||
| else: | ||
| self.port = None | ||
| self.port = int(port) if port is not None else None |
There was a problem hiding this comment.
For what it's worth, the first assignment could just be annotated with Optional[int], but this is fine too.
| @@ -284,7 +281,7 @@ def __str__(self): | |||
| else: | |||
| buf += self.addr | |||
|
|
|||
| if self.port: | |||
| if self.port is not None: | |||
There was a problem hiding this comment.
This isn't necessary. Why did you make this change?
| @@ -196,56 +196,69 @@ def configure_pro(token, enable=None): | |||
|
|
|||
| try: | |||
| enable_resp = json.loads(enable_stdout) | |||
| handle_enable_errors(enable_resp) | |||
There was a problem hiding this comment.
The annotation on this function is still wrong. json.loads() could return more types than this.
| msg = f'Failure of type `{err["type"]}`: {err["message"]}' | ||
| util.logexc(LOG, msg) | ||
| def handle_enable_errors(enable_resp: Union[List[Any], Dict[str, Any]]): | ||
| if isinstance(enable_resp, list): |
There was a problem hiding this comment.
Why special case the list type but still use a conditional branch to handle all other types? Both do the same thing: log a warning and exit.
There was a problem hiding this comment.
To minimize unnecessary diff size and eliminate unnecessary branches, I would recommend this:
if not isinstance(enable_resp, list):
LOG.warn(...)
return
| else: | ||
| msg = f'Failure of type `{err["type"]}`: {err["message"]}' | ||
| util.logexc(LOG, msg) | ||
| def handle_enable_errors(enable_resp: Union[List[Any], Dict[str, Any]]): |
There was a problem hiding this comment.
This type annotation is inaccurate, and it just silences later errors that need to be addressed. Please remove the new function definition and fix the incorrect handling of types later in this module.
Related to 5445
Part of the Onboarding.
✓ cloudinit.config.cc_power_state_change
✓ cloudinit.config.cc_rsyslog
✓ cloudinit.config.cc_ubuntu_pro
✓ tests/unittests/config/test_cc_rsyslog
✓ tests/unittests/config/test_cc_ubuntu_pro
✓ests/unittests/config/test_cc_power_state_change
Proposed Commit Message
Additional Context
Test Steps
Merge type