search

dmacvicar / terraform-provider-libvirt

Terraform provider to provision infrastructure with Linux's KVM using libvirt
Apache License 2.0
1.59k stars 458 forks source link

Remove forgotten code in randomMacAddress #799

Closed Bischoff closed 10 months ago

Bischoff commented 3 years ago

This PR solves issue #795 by removing leftover code.

Bischoff commented 3 years ago

Ping @MalloZup and @Isma399 .

Bischoff commented 3 years ago

@Bischoff thx for this I would like however that we document better in the code, maybe with an url or doc all this magic prefix somehow

I would love that too. So far I failed to find such a thing, but maybe I just missed it... @Isma399 , do you have better insights?

Isma399 commented 3 years ago

No, I have looked for documentation and didn't find anything. Neither in http://standards-oui.ieee.org/oui.txt, nor in qemu documentation.

Bischoff commented 3 years ago

Then, I would suggest just merging like this. This is apparently the kind of facts that everyone uses but that is documented nowhere.

dmacvicar commented 3 years ago

We will skip this for this release. I need some documentation and testcases.

MalloZup commented 3 years ago

I think for this PR we need to find out evidence in the codebase https://gitlab.com/libvirt/libvirt and document why we are doing this change and documenting it + a testcase for this..

Of course previously the behavior was wrong or without technical facts, and this PR fix it somehow but we need to close the gap and make the fix correctly with a doc comment or link something that justify the change

Thx @Bischoff and @Isma399 for PR , would be great if one of you could help us to finish this :pray: :sunflower:

Bischoff commented 3 years ago

Test and documentation should have been added to the initial PR that introduced the problem. I won't add them here, it's out of scope of the bug I am trying to fix.