Closed fossedihelm closed 10 months ago
Hi @fossedihelm. Thanks for your PR.
I'm waiting for a k8snetworkplumbingwg member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test
on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.
Once the patch is verified, the new status will be reflected by the ok-to-test
label.
I understand the commands that are listed here.
@RamLavi Can you have a look please? Thanks
@maiqueb: changing LGTM is restricted to collaborators
Maybe the correct thing to do is to create a MAC address -
net.HardwareAddr
- usingnet.ParseMAC
and then get the string from it (whatever that is) ?
@maiqueb That's a great idea! IMHO we can also skip the err
from ParseMAC function, since it has already been validated. WDYT?
Maybe the correct thing to do is to create a MAC address -
net.HardwareAddr
- usingnet.ParseMAC
and then get the string from it (whatever that is) ?@maiqueb That's a great idea! IMHO we can also skip the
err
from ParseMAC function, since it has already been validated. WDYT?
Fine with me. I mean, there's nothing wrong in being redundant - checking against nil won't take long - but if you're sure that was already validated, all good. I just find it more readable / comforting to see the (ugly) golang pattern of if err != nil ; return err
. It's up to the maintainer.
@maiqueb: changing LGTM is restricted to collaborators
Fine with me. I mean, there's nothing wrong in being redundant - checking against nil won't take long - but if you're sure that was already validated, all good. I just find it more readable / comforting to see the (ugly) golang pattern of
if err != nil ; return err
. It's up to the maintainer.
Yes, I generally agree! In this specific case, handling the error means to create something like this:
func NewMacKey(macAddressDashes string) (macKey, error) {
macAddress, err := net.ParseMAC(macAddressDashes)
if err != nil{
return macKey{}, err
}
return macKey{normalizedMacAddress: macAddress.String()}, nil
}
This is not a problem at all, but this will not allow us to create inline entry in the macMap. And the latter is very useful in tests, in addition to the fact that we should clearly always handle the return error.
Fine with me. I mean, there's nothing wrong in being redundant - checking against nil won't take long - but if you're sure that was already validated, all good. I just find it more readable / comforting to see the (ugly) golang pattern of
if err != nil ; return err
. It's up to the maintainer.Yes, I generally agree! In this specific case, handling the error means to create something like this:
func NewMacKey(macAddressDashes string) (macKey, error) { macAddress, err := net.ParseMAC(macAddressDashes) if err != nil{ return macKey{}, err } return macKey{normalizedMacAddress: macAddress.String()}, nil }
This is not a problem at all, but this will not allow us to create inline entry in the macMap. And the latter is very useful in tests, in addition to the fact that we should clearly always handle the return error.
Maybe it's best to add a comment to the NewMacKey
func indicating it's inputs must be already validated MAC addresses ? And indicate all it does is parse from N mac address formats to "our" definition of canonical format.
This is just a nit. Consider adding it if someone else asks you to change the code.
Maybe it's best to add a comment to the
NewMacKey
func indicating it's inputs must be already validated MAC addresses ? And indicate all it does is parse from N mac address formats to "our" definition of canonical format.This is just a nit. Consider adding it if someone else asks you to change the code.
I put this just before the ParseMAC func:
// we can ignore the error here since the string value has already been validated
but yes, I can insert the description above the func! Thanks, will do!
@maiqueb PTAL
@maiqueb: changing LGTM is restricted to collaborators
/ok-to-test
/hold
let's merge only after stable branches are introduced to KMP
Thanks @RamLavi for the review! I am working on the relevant failing e2e tests. Thank you!
iface.MacAddress
and macKey.String()
-- the first one was upper case, the latter in lower case.MarshalJSON() ([]byte, error)
to macMap struct to allow logger to correctly print the mapthanks for the changes! Big like for the Marshal function added.
Added one more comment.
Thanks @RamLavi for the great suggestion! I addressed the commets. PTAL
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: RamLavi
The full list of commands accepted by this bot can be found here.
The pull request process is described here
/hold cancel
/cherry-pick release-0.42
@RamLavi: new pull request created: #407
A macAddress can be specified with different separators:
Currently, the duplicate mac check works only if the two compared mac address use the same separator. The duplicate check is made looking at the existence of key of the macMap. The keys of the mapMap are the macAddress string.
With this commit, a
macKey
struct is introduced. The latter stores the normalized string of the macAddress and can be used as unique key and identifier of a macAddress.What this PR does / why we need it:
Special notes for your reviewer: Fixes https://issues.redhat.com/browse/CNV-30974
Release note: