added client for setting connection context for passthrough nse #883
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mixaster995
force-pushed
the
master
branch
2 times, most recently
from
37a05ef
to
d453d4a
Compare
Mixaster995
requested review from
denis-tingaikin,
Bolodya1997,
d-uzlov and
glazychev-art
Bolodya1997
reviewed
Apr 30, 2021
Bolodya1997
reviewed
Apr 30, 2021
pkg/networkservice/common/contexttranslation/set_connection_context.go
Outdated
Show resolved
Hide resolved
Bolodya1997
reviewed
Apr 30, 2021
d-uzlov
reviewed
May 4, 2021
d-uzlov
reviewed
May 4, 2021
Mixaster995
force-pushed
the
master
branch
2 times, most recently
from
d41f289
to
bd04ccc
Compare
denis-tingaikin
requested changes
May 5, 2021
| } | ||
|
|
||
| // NewSetConnectionContextClient - returns client that changes connection context to one, provided as parameter or default if nil provided | ||
| func NewSetConnectionContextClient() networkservice.NetworkServiceClient { |
There was a problem hiding this comment.
NewSetConnectionContextClient
Suggested change
| func NewSetConnectionContextClient() networkservice.NetworkServiceClient { | |
| func NewClient() networkservice.NetworkServiceClient { |
| } | ||
|
|
||
| func (s *setConCtxClient) Close(ctx context.Context, conn *networkservice.Connection, opts ...grpc.CallOption) (*empty.Empty, error) { | ||
| return next.Client(ctx).Close(ctx, conn, opts...) |
There was a problem hiding this comment.
Why we do nothing on close?
| // limitations under the License. | ||
|
|
||
| // Package injectconncontext changes connection context in client chain | ||
| package injectconncontext |
There was a problem hiding this comment.
Why we can not store this chain element near the test?
| client.WithName(fmt.Sprintf("nsc-%v", uuid.New().String())), | ||
| client.WithAdditionalFunctionality( | ||
| checkconnection.NewClient(t, func(t *testing.T, conn *networkservice.Connection) { | ||
| assert.True(t, proto.Equal(connCtx, conn.Context)) |
Comment on lines
545
to
546
| checkconnection.NewClient(t, func(t *testing.T, conn *networkservice.Connection) { | ||
| assert.True(t, proto.Equal(connCtx, conn.Context)) |
There was a problem hiding this comment.
I think we need to have two checks:
- Check that contextContext is updated on backward(before translation client).
- Check that contextContext is updated on forward ( after translation client).
Comment on lines
548
to
553
| checkrequest.NewClient(t, func(t *testing.T, request *networkservice.NetworkServiceRequest) { | ||
| if *requestCount > 0 { | ||
| assert.Equal( | ||
| t, | ||
| connCtx.String(), | ||
| request.GetConnection().Context.String(), |
|
I'm a little unclear why this client for setting connection context for passthrough NSE is needed at all... |
|
/cc @Bolodya1997 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Signed-off-by: Mikhail avramenkomihail15@gmail.com
#879