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.

Sign up for GitHub

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

fix untyped-defs on /cloudinit/config & /tests/config/ #5985

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix untyped-defs on /cloudinit/config & /tests/config/ #5985

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8214d7a
GH #5445
Jan 21, 2025
0007f79
update CLA
Jan 21, 2025
f94d4f2
updated for PR# 0007f79
Jan 27, 2025
7864437
added type validation for ubuntu_pro
Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion cloudinit/config/cc_power_state_change.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,10 @@ def givecmdline(pid):
(output, _err) = subp.subp(["procstat", "-c", str(pid)])
line = output.splitlines()[1]
m = re.search(r"\d+ (\w|\.|-)+\s+(/\w.+)", line)
return m.group(2)
if m:
return m.group(2)
else:
return None
else:
return util.load_text_file("/proc/%s/cmdline" % pid)
except IOError:
Expand Down
7 changes: 2 additions & 5 deletions cloudinit/config/cc_rsyslog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,10 +246,7 @@ def __init__(
self.proto = proto

self.addr = addr
if port:
self.port = int(port)
else:
self.port = None
self.port = int(port) if port is not None else None
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth, the first assignment could just be annotated with Optional[int], but this is fine too.


def validate(self):
if self.port:
Expand Down Expand Up @@ -284,7 +281,7 @@ def __str__(self):
else:
buf += self.addr

if self.port:
if self.port is not None:
Copy link
Member

Choose a reason for hiding this comment

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

This isn't necessary. Why did you make this change?

buf += ":%s" % self.port

if self.name:
Expand Down
101 changes: 57 additions & 44 deletions cloudinit/config/cc_ubuntu_pro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
import json
import logging
import re
from typing import Any, List
from typing import Any, Dict, List, Union
from urllib.parse import urlparse

from cloudinit import performance, subp, util
Expand Down Expand Up @@ -196,56 +196,69 @@ def configure_pro(token, enable=None):

try:
enable_resp = json.loads(enable_stdout)
handle_enable_errors(enable_resp)
Copy link
Member

Choose a reason for hiding this comment

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

The annotation on this function is still wrong. json.loads() could return more types than this.


except json.JSONDecodeError as e:
raise RuntimeError(
f"Pro response was not json: {enable_stdout}"
) from e

# At this point we were able to load the json response from Pro. This
# response contains a list of errors under the key 'errors'. E.g.
#
# {
# "errors": [
# {
# "message": "UA Apps: ESM is already enabled ...",
# "message_code": "service-already-enabled",
# "service": "esm-apps",
# "type": "service"
# },
# {
# "message": "Cannot enable unknown service 'asdf' ...",
# "message_code": "invalid-service-or-failure",
# "service": null,
# "type": "system"
# }
# ]
# }
#
# From our pov there are two type of errors, service and non-service
# related. We can distinguish them by checking if `service` is non-null
# or null respectively.

enable_errors: List[dict] = []
for err in enable_resp.get("errors", []):
if err["message_code"] == "service-already-enabled":
LOG.debug("Service `%s` already enabled.", err["service"])
continue
enable_errors.append(err)

if enable_errors:
error_services: List[str] = []
for err in enable_errors:
service = err.get("service")
if service is not None:
error_services.append(service)
msg = f'Failure enabling `{service}`: {err["message"]}'
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]]):
Copy link
Member

Choose a reason for hiding this comment

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

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.

if isinstance(enable_resp, list):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

To minimize unnecessary diff size and eliminate unnecessary branches, I would recommend this:

if not isinstance(enable_resp, list):
    LOG.warn(...)
    return

LOG.warning(
"Unexpected list response from pro enable: %s", enable_resp
)
return
elif isinstance(enable_resp, dict) and "errors" in enable_resp:
# At this point we were able to load the json response from Pro. This
# response contains a list of errors under the key 'errors'. E.g.
#
# {
# "errors": [
# {
# "message": "UA Apps: ESM is already enabled ...",
# "message_code": "service-already-enabled",
# "service": "esm-apps",
# "type": "service"
# },
# {
# "message": "Cannot enable unknown service 'asdf' ...",
# "message_code": "invalid-service-or-failure",
# "service": null,
# "type": "system"
# }
# ]
# }
#
# From our pov there are two type of errors, service and non-service
# related. We can distinguish them by checking if `service` is non-null
# or null respectively.
enable_errors: List[Dict[str, Any]] = []
for err in enable_resp.get("errors", []):
if err["message_code"] == "service-already-enabled":
LOG.debug("Service `%s` already enabled.", err["service"])
continue
enable_errors.append(err)

if enable_errors:
error_services: List[str] = []
for err in enable_errors:
service = err.get("service")
if service is not None:
error_services.append(service)
msg = f'Failure enabling `{service}`: {err["message"]}'
else:
msg = f'Failure of type `{err["type"]}`: {err["message"]}'
util.logexc(LOG, msg)

raise RuntimeError(
"Failure enabling Ubuntu Pro service(s): "
+ ", ".join(error_services)
raise RuntimeError(
"Failure enabling Ubuntu Pro service(s): "
+ ", ".join(error_services)
)
else:
LOG.warning(
"Unexpected response format from pro enable: %s", enable_resp
)


Expand Down
6 changes: 0 additions & 6 deletions pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,6 @@ module = [
"cloudinit.config.cc_ca_certs",
"cloudinit.config.cc_growpart",
"cloudinit.config.cc_ntp",
"cloudinit.config.cc_power_state_change",
"cloudinit.config.cc_rsyslog",
"cloudinit.config.cc_ubuntu_pro",
"cloudinit.config.modules",
"cloudinit.distros",
"cloudinit.distros.alpine",
Expand Down Expand Up @@ -134,19 +131,16 @@ module = [
"tests.unittests.config.test_cc_mcollective",
"tests.unittests.config.test_cc_mounts",
"tests.unittests.config.test_cc_phone_home",
"tests.unittests.config.test_cc_power_state_change",
"tests.unittests.config.test_cc_puppet",
"tests.unittests.config.test_cc_resizefs",
"tests.unittests.config.test_cc_resolv_conf",
"tests.unittests.config.test_cc_rh_subscription",
"tests.unittests.config.test_cc_rsyslog",
"tests.unittests.config.test_cc_runcmd",
"tests.unittests.config.test_cc_snap",
"tests.unittests.config.test_cc_ssh",
"tests.unittests.config.test_cc_timezone",
"tests.unittests.config.test_cc_ubuntu_autoinstall",
"tests.unittests.config.test_cc_ubuntu_drivers",
"tests.unittests.config.test_cc_ubuntu_pro",
"tests.unittests.config.test_cc_update_etc_hosts",
"tests.unittests.config.test_cc_users_groups",
"tests.unittests.config.test_cc_wireguard",
Expand Down
2 changes: 1 addition & 1 deletion tests/unittests/config/test_cc_power_state_change.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def test_empty_mode(self):
self.assertRaises(TypeError, psc.load_power_state, cfg, self.dist)

def test_valid_modes(self):
cfg = {"power_state": {}}
cfg: dict = {"power_state": {}}
for mode in ("halt", "poweroff", "reboot"):
cfg["power_state"]["mode"] = mode
check_lps_ret(psc.load_power_state(cfg, self.dist), mode=mode)
Expand Down
4 changes: 2 additions & 2 deletions tests/unittests/config/test_cc_rsyslog.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ def test_install_rsyslog_on_freebsd(self, m_which):
with mock.patch.object(
cloud.distro, "install_packages"
) as m_install:
handle("rsyslog", {"rsyslog": config}, cloud, None)
handle("rsyslog", {"rsyslog": config}, cloud, [])
m_which.assert_called_with(config["check_exe"])
m_install.assert_called_with(config["packages"])

Expand All @@ -356,6 +356,6 @@ def test_no_install_rsyslog_with_check_exe(self, m_which, m_isbsd):
m_isbsd.return_value = False
m_which.return_value = "/usr/sbin/rsyslogd"
with mock.patch.object(cloud.distro, "install_packages") as m_install:
handle("rsyslog", {"rsyslog": config}, cloud, None)
handle("rsyslog", {"rsyslog": config}, cloud, [])
m_which.assert_called_with(config["check_exe"])
m_install.assert_not_called()
25 changes: 15 additions & 10 deletions tests/unittests/config/test_cc_ubuntu_pro.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,19 +57,24 @@ def fake_uaclient(mocker):
m_uaclient = mock.Mock()

sys.modules["uaclient"] = m_uaclient
mock_exceptions_module = mock.Mock()

# Exceptions
_exceptions = namedtuple(
"exceptions",
Exceptions = namedtuple(
"Exceptions",
[
"UserFacingError",
"AlreadyAttachedError",
],
)(
)
mock_exceptions_module.UserFacingError = FakeUserFacingError
mock_exceptions_module.AlreadyAttachedError = FakeAlreadyAttachedError
sys.modules["uaclient.api.exceptions"] = mock_exceptions_module
_exceptions = Exceptions(
FakeUserFacingError,
FakeAlreadyAttachedError,
)
sys.modules["uaclient.api.exceptions"] = _exceptions
return _exceptions


@pytest.mark.usefixtures("fake_uaclient")
Expand Down Expand Up @@ -834,7 +839,7 @@ def test_handle_attach(
caplog,
):
"""Non-Pro schemas and instance."""
handle("nomatter", cfg=cfg, cloud=cloud, args=None)
handle("nomatter", cfg=cfg, cloud=cloud, args=[])
for record_tuple in log_record_tuples:
assert record_tuple in caplog.record_tuples
if maybe_install_call_args_list is not None:
Expand Down Expand Up @@ -961,7 +966,7 @@ def test_handle_auto_attach_vs_attach(
m_auto_attach.side_effect = auto_attach_side_effect

with expectation:
handle("nomatter", cfg=cfg, cloud=cloud, args=None)
handle("nomatter", cfg=cfg, cloud=cloud, args=[])

for record_tuple in log_record_tuples:
assert record_tuple in caplog.record_tuples
Expand Down Expand Up @@ -1006,7 +1011,7 @@ def test_no_fallback_attach(
enable or disable pro auto-attach.
"""
m_should_auto_attach.return_value = is_pro
handle("nomatter", cfg=cfg, cloud=self.cloud, args=None)
handle("nomatter", cfg=cfg, cloud=self.cloud, args=[])
assert not m_attach.call_args_list

@pytest.mark.parametrize(
Expand Down Expand Up @@ -1061,7 +1066,7 @@ def test_handle_errors(self, cfg, match):
"nomatter",
cfg=cfg,
cloud=self.cloud,
args=None,
args=[],
)

@mock.patch(f"{MPATH}.subp.subp")
Expand All @@ -1087,7 +1092,7 @@ def test_pro_config_error_invalid_url(self, m_subp, caplog):
"nomatter",
cfg=cfg,
cloud=self.cloud,
args=None,
args=[],
)
assert not caplog.text

Expand All @@ -1107,7 +1112,7 @@ def test_fallback_to_attach_no_token(
"nomatter",
cfg=cfg,
cloud=self.cloud,
args=None,
args=[],
)
assert [] == m_subp.call_args_list
assert (
Expand Down
1 change: 1 addition & 0 deletions tools/.github-cla-signers
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,7 @@ metajiji
michaelrommel
mitechie
MjMoshiri
MostafaTarek124eru
mxwebdev
nazunalika
nelsonad-ops
Expand Down