-
Notifications
You must be signed in to change notification settings - Fork 376
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
[DO NOT MERGE] Gen1 Request Forwarding to Gen2 #10535
Conversation
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add the timeout to avoid issues. Check the error response, but other than that, it looks good. Nice work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Just a few comments
proxyTimeout: config.phoneNumberPrivacy.odisServices.timeoutMilliSeconds, | ||
}) | ||
|
||
const originalPath = req.path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you just trying to get the endpoint? You should be able to just use req.url for that
case 'mainnet': | ||
// XXX (soloseng):URL may need to be updated after gen2 function is created on mainnet | ||
destinationUrl = | ||
'https://us-central1-celo-pgpnp-mainnet.cloudfunctions.net/combinerGen2' + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like you forgot to add strippedPath here
} | ||
proxy.on('error', (_) => { | ||
logger.error('Error in Proxying request to Combiner.') | ||
res.status(500).json({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this override the actual error returned by the gen2 combiner? I'd rather just pass that along to the end user if possible
moved to celo-org/social-connect#46 |
Description
Turn
gen1
cloud function into a proxy between client andgen2
, when enabled by config.