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

Use slot number to report MAC address #730

Merged
merged 2 commits into from
Feb 9, 2017
Merged

Conversation

jarrpa
Copy link
Contributor

@jarrpa jarrpa commented Feb 2, 2017

Use a network device's slot number to detect and store the generated MAC address if one hasn't already been provided.

This accommodates scenarios where multiple devices are defined on the same network.

Closes: #645

Copy link
Contributor

@electrofelix electrofelix left a comment

Choose a reason for hiding this comment

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

Conceptually this works but it relies on the order of the devices returned from listing vagrant networking interfaces must match the order created in the libvirt XML. I believe that vagrant-libvirt supports setting :libvirt__adapter to insert declared devices in an order different to that they were declared in the vagrant definition thus violating that assumption and meaning we cannot rely on the index returned from interating the network interfaces returned by vagrant matching the position in the XML defined.

Some tests would confirm this either way, which lack of time to write was blocking me from doing anything one issue.

@jarrpa
Copy link
Contributor Author

jarrpa commented Feb 3, 2017

....if so, then this seems extremely broken. Is it possible to create a domain with two devices and number one of them as adapter 3? Then you'd have an empty slot between, it would seem, and that should cause an error when trying to iterate through each_with_index. Unless I'm still too new to Ruby.

@pronix
Copy link
Member

pronix commented Feb 4, 2017

this patch will broke existed behaviour, please update it.

@electrofelix
Copy link
Contributor

@jarrpa agreed leaving a hole seems incorrect but when you have multiple networks defined the ability to explicitly control device and network order is supported so need to retain it while fixing here.

Perhaps it's not sufficiently useful that it could be deprecated and removed in the future, but would have to go through a period where users are notified of the plan to remove support.

@jarrpa
Copy link
Contributor Author

jarrpa commented Feb 6, 2017

@pronix @electrofelix Okay, changed it up a bit, going off the comment made here. Also cleaned up a little bit of logic to make the change easier to read.

@pronix pronix merged commit 5ed93bd into vagrant-libvirt:master Feb 9, 2017
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.

None yet

3 participants