Skip to content

Conversation

ydirson
Copy link
Contributor

@ydirson ydirson commented Jun 11, 2025

This PR collects all the small improvements that ended up in #311 and #316, and are independent enough to be useful to merge before those get settled.

ydirson added 10 commits June 11, 2025 13:26
This will detect any lack of parameter value more rapidly.

Signed-off-by: Yann Dirson <[email protected]>
- adding it behind the scene makes it more difficult to write tests for IPv6
- only needed for install, not upgrade or restore

Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
This is useful for a lambda passed to @pytest.mark.answerfile, where in
some variants of a test we want to add an element, but nothing in other
variants (eg. a <raid> block)

Signed-off-by: Yann Dirson <[email protected]>
This is a preparation for type hint addition, where we cannot mutate
in-place a variable to another type: we have to build an object of the
correct return type incrementally.

Signed-off-by: Yann Dirson <[email protected]>
Issue raised by type checkers.

Signed-off-by: Yann Dirson <[email protected]>
Type checkers today are unable to determine that `defn` does not contain
an `attrib` member, this prevents them from checking our dict would
provide compatible data (which is does not).

Signed-off-by: Yann Dirson <[email protected]>
Comment on lines 434 to +435
marker = request.node.get_closest_marker("default_vm")
default_vm = marker.args[0] if marker is not None else None
if default_vm is not None:
logging.info(">> No VM specified on CLI. Using default: %s." % default_vm)
ref = default_vm
if marker 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.

it could be a good place to use the famous walrus operator:

        if (marker := request.node.get_closest_marker("default_vm")) is not None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, it's already in 3.8 :)
hm, I have mixed feelings in this case :)

Copy link
Member

@stormi stormi left a comment

Choose a reason for hiding this comment

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

LGTM. Before merging we likely need to run at least some testsuites to avoid surprise unwanted failures in CI.

ydirson added 7 commits June 11, 2025 18:35
PEP585 implemented in python 3.9 allows to subscript `list` and
`dict`, and doing so even with 3.8 is not flagged by checkers, so devs
end up using it and get flagged by the CI.

Since 3.7 `from __future__ import annotations` allows to defer
evaluation of annotations, so any use of collection subscripting in an
annotation gets not checked by the interpreter any more, and we can
use the more comfortable syntax.

Signed-off-by: Yann Dirson <[email protected]>
Signed-off-by: Yann Dirson <[email protected]>
The helper_vm_with_plugged_disk fixture for tune_firstboot was only able
to attach a single disk.  For RAID support we need several disks, and that
fixture would fail.

Signed-off-by: Yann Dirson <[email protected]>
Using a temporary variable is unnecessary and hurts readability.

Also use logger formatting as designed.

Signed-off-by: Yann Dirson <[email protected]>
Code will be more readable when we start manipulating other info about
system disks.

Signed-off-by: Yann Dirson <[email protected]>
Well, it was indeed working "by chance", as calling readlink on a non-link
returns empty, which instead of getting [ to return non-zero because
empty string is not "busybox" got it to return non-zero because the
comparison operator had no first operand.

Signed-off-by: Yann Dirson <[email protected]>
@ydirson ydirson force-pushed the installer-features/prereqs branch from d015bf5 to feebb8a Compare June 11, 2025 16:54
@ydirson
Copy link
Contributor Author

ydirson commented Jun 12, 2025

LGTM. Before merging we likely need to run at least some testsuites to avoid surprise unwanted failures in CI.

Ran tests/misc/test_vm_basic_operations.py. Besides, with full raid1 branch tests/install/ was tested previously.

@ydirson ydirson merged commit 0aa175b into master Jun 12, 2025
7 checks passed
@ydirson ydirson deleted the installer-features/prereqs branch June 12, 2025 09:56
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