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.

Sign up for GitHub

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

Jump to bottom

Add DialOpts and CallOpts to API client. #6301

Merged
merged 3 commits into from
Apr 7, 2021
Merged

Add DialOpts and CallOpts to API client. #6301

merged 3 commits into from
Apr 7, 2021

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented Apr 2, 2021 •
edited
Loading

Changes:

  • Added client.Config.DialOpts.
  • Added client.callOpts which can be used through WithCallOptions. I chose this route for a few reasons:
    • It's a pretty clean way to give the ability to set call options without cluttering each request with an extra parameter.
    • Adding a CallOptions parameter to each requests break the auth.ClientI interface, which makes it more work than it's worth.
    • The main issue is that it makes a shallow copy of the client, which shares a connection and other fields. However this isn't problem if just use this for chaining (as inteded), which will cause the copy to get garbage collected after the call.

Fixes #6276

@Joerger Joerger marked this pull request as ready for review April 2, 2021 23:17
@Joerger Joerger requested a review from awly April 2, 2021 23:17
@Joerger Joerger mentioned this pull request Apr 5, 2021
Merged
@marshall-lee
Copy link
Contributor

@Joerger Hey you forgot to pass callOpts to WatchEvents in streamwatcher.go and also in a couple of places. Please check all the places where Client.grpc stub is referenced

@Joerger Joerger force-pushed the joerger/client-grpc-options branch from 0e22598 to e5ea3a0 Compare April 6, 2021 23:27
@Joerger
Copy link
Contributor Author

Joerger commented Apr 6, 2021

@marshall-lee Fixed, thanks.

awly
awly approved these changes Apr 6, 2021
View reviewed changes
@Joerger Joerger force-pushed the joerger/client-grpc-options branch from e5ea3a0 to 6d58503 Compare April 7, 2021 01:32
russjones
russjones reviewed Apr 7, 2021
View reviewed changes
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

russjones
russjones approved these changes Apr 7, 2021
View reviewed changes
Copy link
Contributor

@russjones russjones left a comment

Choose a reason for hiding this comment

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

Bot.

@Joerger Joerger force-pushed the joerger/client-grpc-options branch from 6d28a86 to 99c8782 Compare April 7, 2021 17:06
@Joerger
Add DialOpts to client.Config
cb7d8c1 
Add callOpts to client and client.WithCallOptions.
@Joerger Joerger force-pushed the joerger/client-grpc-options branch from 99c8782 to cb7d8c1 Compare April 7, 2021 17:55
awly
awly approved these changes Apr 7, 2021
View reviewed changes
@Joerger Joerger force-pushed the joerger/client-grpc-options branch from 3482baa to 809e6c6 Compare April 7, 2021 21:05
@Joerger Joerger enabled auto-merge (squash) April 7, 2021 21:10
@Joerger Joerger merged commit 5e3f235 into master Apr 7, 2021
@Joerger Joerger deleted the joerger/client-grpc-options branch April 7, 2021 21:23
Joerger added a commit that referenced this pull request Apr 7, 2021
@Joerger
Add DialOpts and CallOpts to API client. (#6301)
a560b13 
* Add DialOpts to client.Config.

* Add callOpts to client and client.WithCallOptions.

* Refactor use of atomic closedFlag.
Joerger added a commit that referenced this pull request Apr 7, 2021
@Joerger
Add DialOpts and CallOpts to API client. (#6301) (#6312)
2d41d36
@webvictim webvictim mentioned this pull request May 5, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@awly awly awly approved these changes

@russjones russjones russjones approved these changes

@klizhentas klizhentas Awaiting requested review from klizhentas

@r0mant r0mant Awaiting requested review from r0mant

Assignees
No one assigned
Labels
backport-required
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support GRPC connect/call options in teleport/api
4 participants
@Joerger @marshall-lee @awly @russjones