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.

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

apt: fix invocation of setup_overlay in dry-run mode #2075

Merged
merged 1 commit into from
Sep 6, 2024

Conversation

ogayot
Copy link
Member

@ogayot ogayot commented Sep 5, 2024

setup_overlay() expects a list of lower directories. That said, we have one invocation in dry-run mode where we pass it a path (which is a string) directly instead of wrapping the path in a list with one element.

The dry-run implementation of setup_overlay() accesses the first element of the list using lowers[0], and expects a path out of it. But when lowers is a string, lowers[0] returns the first character of string, which usually is "/" and therefore corresponds to a valid path.

It makes the code accidentally work, but use a path that is not the one we expected.

setup_overlay() expects a list of lower directories. That said, we have
one invocation in dry-run mode where we pass it a path (which is a
string) directly instead of wrapping the path in a list with one
element.

The dry-run implementation of setup_overlay() accesses the first element
of the list using lowers[0], and expects a path out of it. But when
lowers is a string, lowers[0] returns the first character of string,
which usually is "/" and therefore corresponds to a valid path.

It makes the code accidentally work, but use a path that is not the one
we expected.

Signed-off-by: Olivier Gayot <olivier.gayot@canonical.com>
@ogayot
Copy link
Member Author

ogayot commented Sep 5, 2024

It also makes mypy a little bit happier \o/

@@ -675 +674,0 @@
-subiquity/server/apt.py:378: error: Argument 1 to "setup_overlay" of "Mounter" has incompatible type "str | Any"; expected "list[Mountpoint | str | OverlayMountpoint]"  [arg-type]
@@ -2008 +2007 @@
-Found 1911 errors in 142 files (checked 334 source files)
+Found 1910 errors in 142 files (checked 334 source files)

@ogayot ogayot merged commit 3024d43 into canonical:main Sep 6, 2024
12 checks passed
@ogayot ogayot deleted the lowerdir-dry-run branch September 6, 2024 17:25
@mwhudson
Copy link
Collaborator

Thanks. If you can think of a more elegant way of handling dry-run mode for all this stuff, I am all ears ...

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