Allow cns to register node if no NCs are present

[pjohnst5]

Reason for Change:
Allow CNS to register the node if no NCs are present

Issue Fixed:
Livesite issue 13242071

Validation
#unitTests and,
When NCs are not present, CNS allows re-registration of node:

When NCs are present, CNS denies re-registration of node:

[pjohnst5]

What do you think @rbtr about these go-lint failures?
I think it's because there's a lock object in CNS
Error: copylocks: range var tt copies lock:
Error: copylocks: assignment copies lock value to tt:

and then this one is because I'm trying to do just in-line structs:
Error: Error return value of encoding/json.Marshal is not checked (errchkjson)
https://github.com/Azure/azure-container-networking/pull/1232/files#diff-053ea523f14080c985c2992e7a397d824ced2e6eaa58dada5886438ae58d8111R236

Do you think I could just merge it in as is 😅 ?

[rbtr]

yeah, it's mad because you're not supposed to copy a mutex and you're making a local copy of that tt struct which has the restservice which has a mutex. should be fine to throw a //nolint and an explanation of why at the end of those lines

[pjohnst5]

Does the //nolint go at the end of the line itself? Or at the top of the file?

[rbtr]

can you one-line this with an ||? or maybe it should be a switch?

Suggested change

	if len(service.state.ContainerStatus) == 0 {
	if len(service.state.ContainerIDByOrchestratorContext) == 0 {
	if len(service.state.ContainerStatus) == 0 || len(service.state.ContainerIDByOrchestratorContext) == 0 {

[pjohnst5]

I would probably && it instead, so that both things need to be empty for it to say it's empty

[pjohnst5]

I put an && @rbtr so it's one line now

[rbtr]

Does the //nolint go at the end of the line itself? Or at the top of the file?

end of line. ex:

tt := tt //nolint:copylocks // this mutex copy is to keep a local reference to this variable in the test func closure, and is ok

//nolint:<the linter to ignore> // <the reason for the nolint>

[pjohnst5]

end of line. ex:

Thanks @rbtr, linter seems happy now

[rbtr]

lgtm, but left a couple comments that for things you could improve if you feel like it

[rbtr]

just a thought, this pattern is fundamentally

if (bool) { return false }
return true

which is really

return !bool

[pjohnst5]

Ah I didn't think about that

[rbtr]

i know you didn't change this, but this fallthrough pattern is very unusual in go. if all of these switch tests should actually execute the same case, it's better to do:

case cns.ServiceFabric, cns.Kubernetes, cns.KubernetesCRD, [... other tests ...]:
    // the case

[pjohnst5]

Good point