Skip to content

Add to Gribi Get() tests validation that the correct status is returned#255

Open
gggsmith wants to merge 1 commit into
openconfig:mainfrom
gggsmith:_cl_out_721346682
Open

Add to Gribi Get() tests validation that the correct status is returned#255
gggsmith wants to merge 1 commit into
openconfig:mainfrom
gggsmith:_cl_out_721346682

Conversation

@gggsmith

Copy link
Copy Markdown

The test verifies for Gribi Modify call that after receiving FIB_ACK programming status, Gribi Get calls return FIB_PROGRAMMED status for programmed entries. This is part of Gribi specification https://github.com/openconfig/gribi/blob/master/doc/specification.md#4133-life-cycle-of-an-aft-operation

@robshakir

Copy link
Copy Markdown
Member

@gggsmith Happy to take a look at this PR -- can we make the build check pass?

Thanks,
r.

Comment thread chk/chk.go
Comment on lines +389 to +392
if ipv4, ok := ni.ipv4[v.Ipv4.GetPrefix()]; !ok {
t.Fatalf("did not find entry, did not find ipv4: %s, got: %s\n", v.Ipv4, getres)
} else {
programmedStatusMatch(t, ipv4, wantProto)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
if ipv4, ok := ni.ipv4[v.Ipv4.GetPrefix()]; !ok {
t.Fatalf("did not find entry, did not find ipv4: %s, got: %s\n", v.Ipv4, getres)
} else {
programmedStatusMatch(t, ipv4, wantProto)
ipv4, ok := ni.ipv4[v.Ipv4.GetPrefix()]
if !ok {
t.Fatalf("did not find entry, did not find ipv4: %s, got: %s\n", v.Ipv4, getres)
}
programmedStatusMatch(t, ipv4, wantProto)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggest this layout here and above to avoid the indentation of the other case -- since the !ok case will exit via Fatalf we don't really need the else.

Comment thread compliance/get.go
WithIndex(1).
WithIPAddress("192.0.2.3"))
WithIPAddress("192.0.2.3").
WithFIBProgrammed(wantACK))

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will lead -- in some cases -- to having WithFIBProgrammed(RIB_PROGRAMMED) which seems erroneous right? i.e., we will never have a fib_status field return RIB_PROGRAMMED right?

Instead, should we either have WithProgrammedStatus() which could take arguments of FIB ACKed or RIB ACKed? Or alternatively, WithFIBProgrammedStatus which only allows for FIB ACK or failed?

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.

2 participants