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

Document all environment variables in README.md #2023

Open
halfwhole opened this issue Oct 10, 2022 · 7 comments
Open

Document all environment variables in README.md #2023

halfwhole opened this issue Oct 10, 2022 · 7 comments
Assignees
Labels
documentation Improvements or additions to documentation P3 Work on it after P1 & P2 are cleared

Comments

@halfwhole
Copy link
Collaborator

halfwhole commented Oct 10, 2022

Describe the issue

Not all environment variables are documented in the readme. For example, in src/server/config.ts, the optional variables SALT_ROUNDS, OTP_EXPIRY, REDIRECT_EXPIRY, and STATISTICS_EXPIRY are not documented anywhere.

The sections with environment variables (e.g. server, deployment) do not exactly separate them out correctly either (see #2017 (comment))

@halfwhole halfwhole added documentation Improvements or additions to documentation P3 Work on it after P1 & P2 are cleared labels Oct 10, 2022
@c00kie123
Copy link

Is this still open?

@halfwhole
Copy link
Collaborator Author

hello, the issue is still open! please feel free to work on it if you'd like, though responses from our team may be delayed for the month of January, as we're all working on other projects at the moment 🥲

@chasingtherain
Copy link

Hi @halfwhole

I can work on this, could you assign the ticket to me please?

Thank you.

@halfwhole
Copy link
Collaborator Author

hi @chasingtherain for sure, feel free to work on this and open a PR for it if you'd like!

@chasingtherain
Copy link

Hi @halfwhole

In this comment, I note that some variables are not placed in the correct category:

"the existing env variables under "Server" include e.g. AWS_S3_BUCKET and SENTRY_AUTH_TOKEN that don't seem to belong there, so it seems like documenting env variables wherever is the norm"

Could you advise where AWS_S3_BUCKET and SENTRY_AUTH_TOKEN should be placed at?

@chasingtherain
Copy link

chasingtherain commented Mar 14, 2023

Hi @halfwhole

I have scanned through the different files and found these env variables missing from README

Could you kindly advise on the following:

  1. Whether the variables I identified below should be included in README file

  2. If they should be included, could you advise on the:

    • required and description/value column of these missing variables
    • which table they should be added into (e.g. Server, Batch functions for backups table etc)
  3. There are a number of constant variables in dogstatsd.ts, do they need to be included?
    e.g.

    • export const API_KEY_GENERATE = 'apikey.generate'
    • export const API_KEY_GENERATE_TAG_IS_NEW = 'isnew'
    • export const ERROR_UNHANDLED_REJECTION = 'error.unhandled_rejection'
    • export const MALICIOUS_ACTIVITY_FILE = 'malicious_activity.file'
  4. Combining the question from comment above: Where AWS_S3_BUCKET and SENTRY_AUTH_TOKEN should be placed at?

Thank you.

Variables missing from Server table

Environment Variable Required Description/Value
SALT_ROUNDS No No. of iterations of key derivation function used to generate salt and final hash. Defaults to 10.
OTP_EXPIRY No Defaults to 300000ms (5 mins)
REDIRECT_EXPIRY No Defaults to 300000ms (5 mins)
STATISTICS_EXPIRY No Defaults to 300000ms (5 mins)

Variables found in ci.yml that are not found in README

Environment Variable Required Description/Value Which Table should var be placed in?
SERVERLESS_SERVICE TBD TBD TBC

Variables found in .gitpod.Dockerfile that are not found in README

Environment Variable Required Description/Value Which Table should var be placed in?
PORT TBD Defaults to 8080. TBC
DEBUG TBD Defaults to 1. TBC
SERVICES TBD AWS services to emulate.Defaults to s3. TBC
DATA_DIR TBD Directory Localstack saves data internally. Defaults to /tmp/localstack/data. TBC
HOSTNAME_EXTERNAL TBD Defaults to localstack TBC
AWS_BUCKET_NAME TBD Defaults to local-bucket TBC
BUCKET_ENDPOINT TBD Defaults to http://localhost:4566 TBC
PORT_WEB_UI TBD Defaults to 8055. TBC
SES_HOST TBD Defaults to localhost. TBC
SES_PORT TBD Defaults to 1025. TBC

@halfwhole
Copy link
Collaborator Author

halfwhole commented Mar 16, 2023

thanks for looking into this issue! as for the variables you've identified in the tables above:

Variables missing from Server table

  • these generally look good! for OTP_EXPIRY, REDIRECT_EXPIRY, and STATISTICS_EXPIRY though, might have a small preference to change 300000ms to 300s, since the server actually interprets the variable value in terms of seconds rather than ms; the description can also be more descriptive as to what the env var actually does, not just what value it defaults to
  • perhaps we could go with something like the following?
    • OTP_EXPIRY: Duration of time in seconds for the OTP to expire from the cache. Defaults to 300 (5 mins)
    • REDIRECT_EXPIRY: Duration of time in seconds for the short to long URL mapping to expire from the cache. Defaults to 300 (5 mins)
    • STATISTICS_EXPIRY: Duration of time in seconds for the statistics counts to expire from the cache. Defaults to 300 (5 mins)

Variables found in ci.yml that are not found in README

  • doubt that it's necessary to include SERVERLESS_SERVICE, since the value is already set for us in ci.yml (e.g. see the line echo SERVERLESS_SERVICE=gogovsg >> $GITHUB_ENV;), and so it isn't something we have to configure ourselves when setting up the deployment pipeline

Variables found in .gitpod.Dockerfile that are not found in README

  • for most of the variables identified in the table, we likely won't need to add them to the README -- they're generally variables internal to localstack, a side service, which should be less relevant as users likely won't want/need to configure this! (there may be an argument for documenting these too, but still it's probably less of a priority)
  • thanks for the catch on SES_HOST and SES_PORT though, which we'll want to add -- they are required variables (see lines 42 and 135 of src/server/config.ts), and they actually default to maildev and 25 respectively (see docker-compose.yml, which is run on local dev; the Gitpod defaults are probably less relevant than local dev)
  • to tell if some env vars are required, can take a look at the logic src/server/config.ts -- for example, it's likely mandatory if the program exits if the env variables are missing, and optional if e.g. || is used to give a default value on the server-side if the env variable is missing
  • to tell whether an env var is required on the server-side or on deployment or elsewhere, you can look at where the env var is used! so e.g. for SES_HOST, it's used in process.env.SES_HOST by the backend server in src/server/config.ts, so it comes under the server

as for dogstatsd.ts, those are not env variables and generally not configurable, we won't have to include them!

as for AWS_S3_BUCKET and SENTRY_AUTH_TOKEN, I think I was wrong when previously saying that these were placed under the wrong section. sorry about that! looking at them again, they appear to be classified correctly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation P3 Work on it after P1 & P2 are cleared
Projects
None yet
Development

No branches or pull requests

3 participants