Skip to content

fluent exports GRIBIEntry so that a test can stage them before adding.#75

Merged
liulk merged 1 commit intoopenconfig:mainfrom
liulk:topic/fluentexportentry
Sep 3, 2021
Merged

fluent exports GRIBIEntry so that a test can stage them before adding.#75
liulk merged 1 commit intoopenconfig:mainfrom
liulk:topic/fluentexportentry

Conversation

@liulk
Copy link
Copy Markdown
Contributor

@liulk liulk commented Aug 27, 2021

As a result, the opproto() and entryproto() methods have been modified
to return a copy rather than sharing a reference, so the caller can
safely modify them for use in different operations.

@liulk liulk requested a review from dan-lepage August 27, 2021 18:50
As a result, the opproto() and entryproto() methods have been modified
to return a copy rather than sharing a reference, so the caller can
safely modify them for use in different operations.
@liulk liulk requested review from sthesayi and removed request for dan-lepage August 30, 2021 19:12
Comment thread fluent/fluent.go
Copy link
Copy Markdown
Contributor

@sthesayi sthesayi left a comment

Choose a reason for hiding this comment

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

Per our discussion and looking at the test case construction and assertion, this change makes more sense. There could be alternative implementations that can avoid exposing GRIBIEntry but that code will look a lot verbose and hard to maintain. Also given this is an interface, I don't see strong reasoning to not approve this change but please do also check with Rob when he returns. Consider this as a provisional approval to unblock test development.

Comment thread fluent/fluent.go
@liulk liulk merged commit 311f603 into openconfig:main Sep 3, 2021
@liulk liulk deleted the topic/fluentexportentry branch September 3, 2021 16:42
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