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

SchemaRegistry: Add http agent argument #108

Merged

Conversation

seriouslag
Copy link
Contributor

Add option to pass in HTTP agent that binds to mappersmith

Add option to pass in HTTP agent that binds to mappersmith
@seriouslag seriouslag force-pushed the feature/schemaRegistry/add-http-agent-arg branch from de7f49e to 27b0b8a Compare April 6, 2021 14:11
@seriouslag
Copy link
Contributor Author

@Nevon any comments on this PR?

Fixes issue #107

src/api/index.ts Outdated
forge({
agent,
}: SchemaRegistryAPIClientArgs): SchemaRegistryAPIClient => {
// TODO: figure out how to only bind agent to host URL
Copy link
Member

Choose a reason for hiding this comment

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

So the way you'd do this is to pass in the gatewayConfigs as a property of the manifest (what gets passed into forge). That way we don't change the gateway configs globally, only for this client.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@seriouslag seriouslag Apr 13, 2021

Choose a reason for hiding this comment

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

I was having issues with the typings of mappersmith (and prettier... forcing a semicolon before parentheses 🤷‍♂️). From looking at the code I believe the latest changes are as mappersmith needs from a config.

Copy link
Member

Choose a reason for hiding this comment

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

Opened a PR in mappersmith to avoid having to do the gnarly casting to any. tulios/mappersmith#211

Let's merge this anyway and then we can hopefully remember to go back and fix this once a new version of mappersmith is out.

@Nevon
Copy link
Member

Nevon commented Apr 13, 2021

This is a very useful change that I'd like to get merged. I only have one comment, to make sure that we don't modify the global gateway config. Other than that it's great.

@seriouslag seriouslag force-pushed the feature/schemaRegistry/add-http-agent-arg branch from 2428a31 to d2354ec Compare April 13, 2021 23:08
src/api/index.ts Outdated
if (agent) {
// gatewayConfigs is not listed as a type on manifest object in mappersmith
;(manifest as any).gatewayConfigs = {
configure: () => agent,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
configure: () => agent,
configure: () => ({ agent }),

See here for an example: https://github.com/tulios/mappersmith#http

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm useless without my types (and coffee...). Good catch! Updated.

@Nevon Nevon merged commit 10d38ff into kafkajs:master Apr 14, 2021
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