Skip to content

Fix label fatboot#513

Merged
OddBloke merged 7 commits into
canonical:masterfrom
TheRealFalcon:fix-label_fatboot
Aug 7, 2020
Merged

Fix label fatboot#513
OddBloke merged 7 commits into
canonical:masterfrom
TheRealFalcon:fix-label_fatboot

Conversation

@TheRealFalcon
Copy link
Copy Markdown
Contributor

Continuation of #500.

Proposed squashed commit message:

Recognize LABEL_FATBOOT labels

Update DataSourceNoCloud and ds-identify to recognize LABEL_FATBOOT labels from blkid.
Also updated associated tests.

LP: #1841466

@TheRealFalcon TheRealFalcon mentioned this pull request Jul 29, 2020
@TheRealFalcon
Copy link
Copy Markdown
Contributor Author

Github's diff view here is a little unfortunate. Beyond incorporating @smoser 's suggested changes, I added a class around the pre-existing pytest tests, added an additional non-BSD test (since we didn't have any), and added an extra FATBOOT_LABEL=... assert to each of the tests. The tests did find an uninitialized variable error, so I fixed that as well.

@smoser and @igalic , since you commented on the last one, I thought you might be interested in my updates here too.

Copy link
Copy Markdown
Collaborator

@igalic igalic left a comment

Choose a reason for hiding this comment

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

how do i recreate these conditions?
to see how this actually looks like in a (free)bsd image?


# lp: #1841466
devlist = util.find_devs_with_freebsd(criteria="LABEL_FATBOOT=A_LABEL")
assert devlist == []
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

has anyone tried booting a freebsd image under these conditions to see what the result looks like, and what we could be looking for?

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.

@igalic Was that one of the BSD people you pinged on the last ticket? Should I ping them here too? If we make an effort to reach out but they're unavailable, do you think the unit test changes are sufficient?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We have roughly 3 *BSD people currently active:

of those, @goneri is the most experienced when it comes to NoCloud

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Aloha,

Thank you @TheRealFalcon and @igalic, I will give a try to the patch.

Copy link
Copy Markdown
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

This LGTM, thanks! Two minor testing notes, but not enough to block landing. (If you care to spend the time, they can come as a follow-up.)

Comment on lines +1017 to +1029
devlist = util.find_devs_with_freebsd()
assert set(devlist) == set([
'/dev/iso9660/config-2', '/dev/msdosfs/EFISYS'])

devlist = util.find_devs_with_freebsd(criteria="TYPE=iso9660")
assert devlist == ['/dev/iso9660/config-2']

devlist = util.find_devs_with_freebsd(criteria="TYPE=vfat")
assert devlist == ['/dev/msdosfs/EFISYS']

# lp: #1841466
devlist = util.find_devs_with_freebsd(criteria="LABEL_FATBOOT=A_LABEL")
assert devlist == []
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It'd be nice to see these tests parameterised.

Comment on lines +1073 to +1089
devlist = util.find_devs_with_netbsd()
assert set(devlist) == set(
["/dev/ld0", "/dev/dk0", "/dev/dk1", "/dev/cd0"]
)

m_subp.side_effect = side_effect_values
devlist = util.find_devs_with_netbsd(criteria="TYPE=iso9660")
assert devlist == ["/dev/cd0"]

m_subp.side_effect = side_effect_values
devlist = util.find_devs_with_netbsd(criteria="TYPE=vfat")
assert devlist == ["/dev/ld0", "/dev/dk0", "/dev/dk1"]

# lp: #1841466
m_subp.side_effect = side_effect_values
devlist = util.find_devs_with_netbsd(criteria="LABEL_FATBOOT=A_LABEL")
assert devlist == ['/dev/ld0', '/dev/dk0', '/dev/dk1', '/dev/cd0']
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These too.

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.

5 participants