Fix Mandatory Attribute Validation

[markafarrell]

What?

• Support checking if mandatory attributes are keys
• Only treat a container as a Choice if it actually is a choice

Why?

• Without this if a key attribute is marked as mandatory then validation will fail
• If we try to treat a container as one but it is not (i.e. s.schema.GetContainer().GetChoiceInfo() returns null) then we will have a null pointer dereference error when running GetChoiceByName(c.Name)

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x1698927]

goroutine 705 [running]:
github.com/sdcio/sdc-protos/sdcpb.(*ChoiceInfo).GetChoiceByName(...)
	/home/runner/go/pkg/mod/github.com/sdcio/sdc-protos@v0.0.46/sdcpb/choice_info_additions.go:4
github.com/sdcio/data-server/pkg/tree.(*sharedEntryAttributes).validateMandatory(0xc0005c1810, {0x1f92800, 0xc000e93b90}, 0xc000b5ad90, 0xc000e8ab38)
	/home/runner/work/data-server/data-server/pkg/tree/sharedEntryAttributes.go:1378 +0x4c7
github.com/sdcio/data-server/pkg/tree.(*sharedEntryAttributes).Validate(0xc0005c1810, {0x1f92800, 0xc000e93b90}, 0xc000b5ad90, 0xc000e8ab38, 0xc000433e50)
	/home/runner/work/data-server/data-server/pkg/tree/sharedEntryAttributes.go:979 +0xa6
github.com/sdcio/data-server/pkg/tree.(*sharedEntryAttributes).Validate.func1({0x1fae540?, 0xc0005c1810?})
	/home/runner/work/data-server/data-server/pkg/tree/sharedEntryAttributes.go:1013 +0x44
created by github.com/sdcio/data-server/pkg/tree.(*sharedEntryAttributes).Validate in goroutine 441
	/home/runner/work/data-server/data-server/pkg/tree/sharedEntryAttributes.go:1017 +0x43a

[steiler]

@markafarrell yes this seems valid and correct.
Is there more comming regarding this topic or why is the PR just a draft?

[markafarrell]

@markafarrell yes this seems valid and correct.
Is there more comming regarding this topic or why is the PR just a draft?

I wanted to add a test case to validate the change before I set the PR to ready.

[markafarrell]

@steiler This is ready for review now

[steiler]

@markafarrell can you take a look at the ygot struct... it contains an ipv6 container, which is not present in the yang definition.
So I assume you need to rerun building the ygot structs from the sdcio test schema.

[markafarrell]

@markafarrell can you take a look at the ygot struct... it contains an ipv6 container, which is not present in the yang definition.
So I assume you need to rerun building the ygot structs from the sdcio test schema.

It is from https://github.com/sdcio/data-server/blob/main/tests%2Fschema%2Fsdcio_model_static_route.yang which was added 3 months ago.

I guess when it was added the ygot make target wasn't run.

I couldn't figure out an elegant way of excluding it.

[steiler]

@markafarrell can you take a look at the ygot struct... it contains an ipv6 container, which is not present in the yang definition.
So I assume you need to rerun building the ygot structs from the sdcio test schema.

It is from https://github.com/sdcio/data-server/blob/main/tests%2Fschema%2Fsdcio_model_static_route.yang which was added 3 months ago.

I guess when it was added the ygot make target wasn't run.

I couldn't figure out an elegant way of excluding it.

Alright, yes sorry, I see.
So we've missed to update the model back then, since the tests where not specifically using the ygot struct.

[steiler]

lgtm

[markafarrell]

lgtm

@steiler Do I need to worry about failing CI?

[steiler]

lgtm

@steiler Do I need to worry about failing CI?

No, the ci is not to be trusted atm. We've encountered issues with the deviation processing, which was finally revealed by the failing integration tests, so we're changing that processing atm. Having a multi repo project with config-server and data-server in separate repos as well as the integration tests also adds some glitches in this case.
The prs for this are kind of ready, just waiting for approval to merge.
So when that's done, we will have 90 percent back on track. The last thing to tackle is then the capability of manually reverting non-revertive intents. Which the tests cover but need to be brought back in thhat new mode of operation.
Ill run the required tests manually. But I'm off still until Monday. Sorry!

[steiler]

OK, so I took a closer look and discovered, that even the unittests where failing which should not have happened.
I discovered that in the afore mentioned PR, the schema, precisely the doublekey element was changed so contain a pattern.
This is only failing now, because it was missed to update the ygot schema struct as mentioned before as part of that PR.
now that you've done it correctly, updating the ygot struct, the issue surfaced and the unittests started failing.
I've now adjusted the doublekey schema slightly and updated the related tests as well as the ygot struct.
So from my perspective this PR is good to go!

[alexandernorth]

lgtm

[steiler]

Thanks @markafarrell

[codecov]

Codecov Report

❌ Patch coverage is 72.72727% with 3 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/tree/sharedEntryAttributes.go	72.72%	2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.