Skip to content

Remove support for empty NI name per gRIBI spec.#122

Merged
robshakir merged 2 commits intomainfrom
empty_network_instance
Mar 3, 2022
Merged

Remove support for empty NI name per gRIBI spec.#122
robshakir merged 2 commits intomainfrom
empty_network_instance

Conversation

@robshakir
Copy link
Copy Markdown
Member

  * (M) compliance/compliance.go
    - Update tests that do not explicitly specify a network instance
      name for the default network instance name to include one.
  * (M) server/server.go
  * (M) server/server_test.go
    - Update reference server implementation to remove the handling
      of an empty network instance name.

See openconfig/gribi#34 for details of this change.

  * (M) compliance/compliance.go
    - Update tests that do not explicitly specify a network instance
      name for the default network instance name to include one.
  * (M) server/server.go
  * (M) server/server_test.go
    - Update reference server implementation to remove the handling
      of an empty network instance name.
@robshakir robshakir requested a review from xw-g March 2, 2022 12:11
Copy link
Copy Markdown
Contributor

@xw-g xw-g left a comment

Choose a reason for hiding this comment

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

There are still places implicitly set the default instance, see below. Do you want to change them too?

@robshakir
Copy link
Copy Markdown
Member Author

Thanks! Somehow I missed the latter one. Let me fix that!

On the first one, that is just creating the default RIB on the server (rather than the expectation in the API) since we do not ingest a configuration on the reference server to tell us what the name should be.

  * (M) server/server.go
    - Remove rewrite of "" to default network instance name.
@robshakir
Copy link
Copy Markdown
Member Author

Thanks - removed the rewrite in modifyEntry. PTAL.

@robshakir robshakir merged commit a12bb20 into main Mar 3, 2022
@robshakir robshakir deleted the empty_network_instance branch March 3, 2022 17:04
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