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

Add environment variable error handling #23

Merged
merged 16 commits into from
Feb 24, 2023
Merged

Conversation

MartinMinkov
Copy link
Contributor

Description

This PR introduces improved error handling for environment variables and better compatibility checks for the Postgres schema. The changes include:

  1. Checking the Postgres schema compatibility with the tables the server uses on startup.
  2. Adding a new environment variable, ENABLE_LOGGING, allows users to specify if they want any logging. If logging is enabled, it will direct to stdout first. If JAEGER_LOGGING is enabled, the environment variables related to that functionality will also be checked.
  3. Adding POST to the allowed CORS methods to support GraphQL.
  4. Minor refactors to improve code quality and maintainability.

Addresses #4 #18

Impact:

These changes will improve the reliability and stability of the server. The improved error handling for environment variables will catch potential issues early on and prevent the server from starting with incorrect configurations. The added compatibility check for the Postgres schema will ensure that the server uses the correct tables, improving data accuracy and consistency. Adding the ENABLE_LOGGING environment variable will give users more control over logging and provide better visibility into the server's operation. Adding POST to the allowed CORS methods will support GraphQL and improve the user experience. The minor refactor will improve the maintainability of the code and make future development easier.

Copy link
Member

@shimkiv shimkiv left a comment

Choose a reason for hiding this comment

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

  • Overall LGTM but left some comments.

src/server.ts Outdated Show resolved Hide resolved
src/server.ts Outdated Show resolved Hide resolved
src/server.ts Show resolved Hide resolved
.env.example Show resolved Hide resolved
@shimkiv
Copy link
Member

shimkiv commented Feb 22, 2023

Also it seems that DB connection string and schema validity is checked for the first host in multi-host connection string provided.

@MartinMinkov MartinMinkov force-pushed the feat/env-error-handling branch from 331faaa to a79b6c3 Compare February 22, 2023 17:30
@MartinMinkov
Copy link
Contributor Author

Also it seems that DB connection string and schema validity is checked for the first host in multi-host connection string provided.

Multi-host checking seems to be more complicated; I will address this in another PR. #29

@shimkiv shimkiv self-requested a review February 24, 2023 12:56
Copy link
Member

@shimkiv shimkiv left a comment

Choose a reason for hiding this comment

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

  • LGTM, left another comment but approved.
  • Thank you.

@MartinMinkov MartinMinkov force-pushed the feat/env-error-handling branch from 2c4b761 to 5616fa2 Compare February 24, 2023 18:26
@MartinMinkov MartinMinkov merged commit 89564e1 into main Feb 24, 2023
@MartinMinkov MartinMinkov deleted the feat/env-error-handling branch February 24, 2023 18:53
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