Skip to content

chore: add type annotations to cloudinit.user_data - #7025

Open
Mohith1612 wants to merge 2 commits into
canonical:mainfrom
Mohith1612:typing-user-data
Open

Mohith1612 wants to merge 2 commits into
canonical:mainfrom
Mohith1612:typing-user-data

Conversation

@Mohith1612

@Mohith1612 Mohith1612 commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Proposed Commit Message

chore: add type annotations to cloudinit.user_data

_do_include only computes include_once_fn when include_once_on is
set, so nest the cached-file lookup inside that branch and fall
through to the fetch when nothing was read. mypy can then see
include_once_fn as a str at the isfile and load_text_file calls.

_explode_archive assigns msg a MIMEText in one branch and a MIMEBase
in the other. MIMEText subclasses MIMEBase, so declare msg as
MIMEBase.

Drop cloudinit.user_data from the mypy override list in
pyproject.toml.

Refs GH-5445

Additional Context

Refs GH-5445. This module is marked as a priority in the issue checklist.

Removing it from the override list surfaces three errors:

user_data.py:233: Argument 1 to "isfile" has incompatible type
                  "Any | None"                          [arg-type]
user_data.py:234: Argument 1 to "load_text_file" has incompatible type
                  "Any | None"                          [arg-type]
user_data.py:303: Incompatible types in assignment (expression has type
                  "MIMEBase", variable has type "MIMEText") [assignment]

The first two. include_once_fn is only assigned inside
if include_once_on:, but the lookup below sat outside that branch, so mypy
still saw it as Optional at the isfile and load_text_file calls. Nesting
the lookup inside the branch it depends on resolves that without any extra
guard.

The fall-through changes from else to if content is None, which is
equivalent: the only assignment in the nested branch is
util.load_text_file(), which is annotated -> str and so never returns
None. The fetch therefore runs in exactly the same cases as before, and a
cached file that happens to be empty still suppresses the fetch, as it did
previously.

I left the later if include_once_on and resp.ok() guard alone. It does not
produce an error and changing it would be noise.

The third. In _explode_archive, msg gets a MIMEText in the text
branch and a MIMEBase in the other, so mypy pins it to MIMEText from the
first assignment. MIMEText is a subclass of MIMEBase and both are already
imported, so a bare msg: MIMEBase declaration is enough. No assignment
changes.

Test Steps

No runtime behaviour change, so no new tests. tests/unittests/test_data.py
already covers the include and include-once paths through _do_include, and
the archive handling through _explode_archive.

$ python -m pytest tests/unittests/test_data.py -q
34 passed

$ tox -e py3
5743 passed, 5 skipped, 13 xfailed, 10 warnings in 173.08s

$ tox -e check_format
ruff: All checks passed!
pylint: Your code has been rated at 10.00/10
black: 595 files would be left unchanged.
isort: Skipped 8 files
mypy: Success: no issues found in 591 source files
congratulations :)

Merge type

  • Squash merge using "Proposed Commit Message"
  • Rebase and merge unique commits. Requires commit messages per-commit each referencing the pull request number (#<PR_NUM>)

@holmanb holmanb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks - one comment.

Comment thread cloudinit/user_data.py Outdated
if include_once_on:
include_once_fn = self._get_include_once_filename(include_url)
if include_once_on and os.path.isfile(include_once_fn):
if include_once_fn is not None and os.path.isfile(include_once_fn):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It would read more cleanly if these were just nested together. I think this is safe since isfile returns a string, so the None check is redundant anyway.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Understood, will update the code!

@Mohith1612

Copy link
Copy Markdown
Contributor Author

Nested as suggested, thanks. The redundant None check is gone:

if include_once_on:
    include_once_fn = self._get_include_once_filename(include_url)
    if os.path.isfile(include_once_fn):
        content = util.load_text_file(include_once_fn)
if content is None:
    try:

One note on the fall-through, since it moved from else to if content is None. That is equivalent because the only assignment in the nested branch is util.load_text_file(), which is annotated -> str and never returns None, so the fetch still runs in exactly the same cases. A cached file that happens to be empty still suppresses the fetch, as before.

Small correction on the reasoning, though it does not change the outcome: os.path.isfile returns a bool, not a string. The thing that made the None check redundant is _get_include_once_filename, which returns an os.path.join result and so is always a str once we are inside the include_once_on branch.

I also rewrote the commit message to describe the current state rather than what the code used to do, per @blackboxsw's note on #7021.

tox -e py3, tox -e check_format and tests/unittests/test_data.py are all green on the updated branch.

@holmanb

holmanb commented Sep 14, 2026

Copy link
Copy Markdown
Member

This has conflicts, please fix.

Two of the three errors are in _do_include. include_once_fn starts as
None and is only assigned when include_once_on is true, but the guard
below tested include_once_on, which mypy cannot connect to
include_once_fn, so it stayed Optional at the isfile and
load_text_file calls.

Test include_once_fn itself instead. _get_include_once_filename
always returns an os.path.join result, so include_once_fn is None
exactly when include_once_on is false and the two conditions are
equivalent.

The third is in _explode_archive, where msg is assigned a MIMEText in
one branch and a MIMEBase in the other. MIMEText subclasses MIMEBase,
so declare msg as MIMEBase.

Drop cloudinit.user_data from the mypy override list in
pyproject.toml.

Refs canonicalGH-5445
Nest the cached-file check inside the include_once_on branch and fall
through on content is None, which drops the redundant None guard.
@Mohith1612

Copy link
Copy Markdown
Contributor Author

Rebased onto main, conflict resolved.

The conflict was in the GH-5445 override list in pyproject.toml. Main had dropped cloudinit.sources.helpers.vultr in #7051 while this branch drops cloudinit.user_data, and the two removals landed adjacent to each other. Both entries are gone in the resolved version, and nothing else in that list moved.

The code change is unchanged from the version you reviewed. tox -e py3, tox -e check_format and tests/unittests/test_data.py are all green on the rebased branch.

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.

3 participants