-
Notifications
You must be signed in to change notification settings - Fork 101
fix(backend): made CARD_SERVICE_URL optional #3600
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
Conversation
🚀 Performance Test ResultsTest Configuration:
Test Metrics:
📜 Logs |
packages/backend/src/index.ts
Outdated
| axios: await deps.use('axios'), | ||
| logger: await deps.use('logger'), | ||
| cardServiceUrl: config.cardServiceUrl | ||
| if (config.cardServiceUrl) { |
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.
Let's remove this if and keep the cardService service defined, otherwise we might get unexpected behaviour if a singleton resolves to undefined.
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.
We can do a NoopCardService (similar to the NoopTelemetryService)
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, for the last comment feel free to push it directly to pos-card-services
| export async function createNoopCardService(): Promise<CardService> { | ||
| return { | ||
| async sendPaymentEvent(_eventDetails) { | ||
| // do nothing |
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.
Let's have a warn log here.
* Made CARD_SERVICE_URL optional * Created cardService container only if cardServiceUrl is set in config * Added noop card service when cardServiceUrl is not provided * Regenerated graphql * Added warning when CARD_SERVICE_URL is not set
Changes proposed in this pull request
CARD_SERVICE_URLis now optionalOpenPaymentsWalletAddressdoes not returncardServiceUrlif it's not setContext
Fixes #3577
Linear #1113
Checklist
fixes #numberuser-docslabel (if necessary)