Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Removed adapters #1149

Closed

Conversation

Mixaster995
Copy link
Contributor

@Mixaster995 Mixaster995 commented Nov 10, 2021

Signed-off-by: Mikhail Avramenko [email protected]

Description

Removed usage of adapters outside test environment

Issue link

closes #1147

Types of changes

  • Bug fix
  • New functionallity
  • Documentation
  • Refactoring
  • CI

@Mixaster995 Mixaster995 force-pushed the fix/remove-adapters branch 29 times, most recently from 93c684f to 58199f6 Compare November 12, 2021 12:37
.golangci.yml Outdated
@@ -35,7 +35,7 @@ linters-settings:
dupl:
threshold: 150
funlen:
Lines: 100
Lines: 105
Copy link
Member

Choose a reason for hiding this comment

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

Can we apply the fix for a file and not change this for all files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed

return next.Client(ctx).Request(ctx, request, opts...)
}

func (s *setLogOptionClient) withFields(ctx context.Context) context.Context {
Copy link
Member

Choose a reason for hiding this comment

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

Can we create a file context.go and put there function withFields?
https://github.com/networkservicemesh/sdk/blob/main/pkg/networkservice/common/setlogoption/server.go#L46-L61

The function can be reused by client and server.

Copy link
Contributor Author

@Mixaster995 Mixaster995 Nov 15, 2021

Choose a reason for hiding this comment

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

ok, done

Comment on lines 49 to 52
fields := make(map[string]interface{})
for k, v := range ctxFields {
fields[k] = v
}
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to make a copy?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed

Comment on lines 26 to 30
fields := make(map[string]interface{})
fields["type"] = "NetworkService"
for k, v := range options {
fields[k] = v
}
Copy link
Member

Choose a reason for hiding this comment

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

what if we already have fields in ctx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, fixed

@edwarnicke
Copy link
Member

@Mixaster995 @denis-tingaikin Is there more work to do here or is this ready to merge?

@Mixaster995
Copy link
Contributor Author

@edwarnicke it is ready, but i think it is better to be frozen until release, because any new change may affect stability, which is our №1 priority. What do you think about that?

Signed-off-by: Mikhail Avramenko <[email protected]>
Signed-off-by: Mikhail Avramenko <[email protected]>
@denis-tingaikin
Copy link
Member

@Mixaster995 , @edwarnicke I think we actually do not need setlogoption pkg. Can we just cut it from sdk?

@Mixaster995
Copy link
Contributor Author

closed in favor of #1186

@Mixaster995 Mixaster995 closed this Dec 2, 2021
@Mixaster995 Mixaster995 deleted the fix/remove-adapters branch December 3, 2021 02:52
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.

Use NewClient instead of NewServerToClient
3 participants