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 support for student project environment variables #632

Merged
merged 20 commits into from
Mar 6, 2025

Conversation

ProchaLu
Copy link
Member

@ProchaLu ProchaLu commented Feb 27, 2025

Prerequisite for

Preflight is failing in Alex Ecommerce Store Project due to missing env variables. This project is the first to include additional environment variables beyond the database ones we teach in the lectures. Since we don't check the .env.example file, Preflight was relying on only the default environment variables, causing it to fail during the dotenv-safe check.

MissingEnvVarsError: The following variables were defined in .env.example but are not present in the environment:
  NEXTAUTH_URL, NEXTAUTH_SECRET, CLOUDINARY_CLOUD_NAME, CLOUDINARY_API_KEY, CLOUDINARY_API_SECRET
Make sure to add them to .env or directly to the environment.

ENOENT: no such file or directory, open '/preflight/project-to-check/.env'
    at config (/preflight/project-to-check/node_modules/.pnpm/[email protected][email protected]/node_modules/dotenv-safe/index.js:31:19)
    at setEnvironmentVariables (file:///preflight/project-to-check/util/config.js:24:3)
    at file:///preflight/project-to-check/ley.config.js:3:1
    at ModuleJob.run (node:internal/modules/esm/module_job:271:25)
    at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:578:26)
    at async exports.load (/preflight/project-to-check/node_modules/.pnpm/@[email protected]/node_modules/@upleveled/ley/lib/util.js:59:11)
    at async /preflight/project-to-check/node_modules/.pnpm/@[email protected]/node_modules/@upleveled/ley/bin.js:11:13 {
  missing: [
    'NEXTAUTH_URL',
    'NEXTAUTH_SECRET',
    'CLOUDINARY_CLOUD_NAME',
    'CLOUDINARY_API_KEY',
    'CLOUDINARY_API_SECRET'
  ],
  sample: '.env.example',
  example: '.env.example'
}

Node.js v22.14.0

This PR updates how Preflight handles additional environment variables. The alpine-postgresql-setup-and-start.sh script now prints the required variables. Preflight captures this output using stdout: pipe, extracts the exact line, and creates environment variables with fake values. With this PR Preflight runs without failing due to missing environment variables.

Preflight output after a successful test

Cloning https://github.com/ProchaLu/drone-preflight-env...
Installing dependencies...
Setting up PostgreSQL database...
initdb: warning: enabling "trust" authentication for local connections
initdb: hint: You can change this by editing pg_hba.conf or using the option -A, or --auth-local and --auth-host, the next time you run initdb.
Running migrations...
Running Preflight...
🚀 UpLeveled Preflight v8.0.6
[STARTED] All changes committed to Git
[STARTED] node_modules/ folder ignored in Git
[STARTED] No extraneous files committed to Git
[STARTED] No secrets committed to Git
[STARTED] Use single package manager
[COMPLETED] No secrets committed to Git
[STARTED] Project folder name matches correct format
[COMPLETED] Project folder name matches correct format
[STARTED] No dependency problems
[STARTED] No unused dependencies
[STARTED] No dependencies without types
[COMPLETED] No extraneous files committed to Git
[STARTED] GitHub repo has deployed project link under About
[COMPLETED] Use single package manager
[STARTED] ESLint
[COMPLETED] node_modules/ folder ignored in Git
[STARTED] Stylelint
[COMPLETED] All changes committed to Git
[STARTED] Prettier
[COMPLETED] GitHub repo has deployed project link under About
[STARTED] ESLint config is latest version
[COMPLETED] No dependencies without types
[COMPLETED] ESLint config is latest version
[STARTED] Stylelint config is latest version
[COMPLETED] No unused dependencies
[COMPLETED] No dependency problems
[STARTED] Preflight is latest version
[COMPLETED] Stylelint
[COMPLETED] Stylelint config is latest version
[COMPLETED] Prettier
[COMPLETED] Preflight is latest version
[COMPLETED] ESLint

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Copy link

codesandbox-ci bot commented Feb 27, 2025

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

karlhorky and others added 7 commits February 27, 2025 19:45
@karlhorky
Copy link
Member

karlhorky commented Mar 1, 2025

Can students use the scripts/alpine-postgresql-setup-and-start.sh file to set environment variables in the Preflight run?

If this works, then we can avoid making a change to Preflight.

@ProchaLu
Copy link
Member Author

ProchaLu commented Mar 5, 2025

Currently I am not allowed to submit a new issue to execa.
Screenshot 2025-03-05 at 12 59 49

Title: Unexpected hanging/blocking behavior when running PostgreSQL

Description:
We've been investigating an issue where Execa does not return control to the parent process after launching a PostgreSQL setup script. The child process starts PostgreSQL successfully, and logs confirm that the database is running, but Execa remains blocked instead of completing execution.

To reproduce the issue, we created a minimal Docker setup where this happens consistently. The setup consists of a Node.js script (parent process) that spawns a Bash script (child process) responsible for initializing and starting PostgreSQL. The expectation is that Execa should return after PostgreSQL starts, but instead, it never does.

The parent script sets up the environment and then calls Execa to execute the child script. The child script initializes the database and starts PostgreSQL using pg_ctl start. At this point, PostgreSQL starts successfully, logs indicate that it is running, but Execa does not return control to the parent script.

[PIPE DEBUG] [Postgres] Prepending volume path to Unix Domain Socket path...
[PIPE DEBUG] [Postgres] Enabling connections on all available IP interfaces...
[PIPE DEBUG] [Postgres] Starting PostgreSQL with pg_ctl...
[PIPE DEBUG] waiting for server to start....
[PIPE DEBUG] 2025-03-05 11:18:02.254 UTC [32] LOG:  starting PostgreSQL 17.4 on aarch64-alpine-linux-musl, compiled by gcc (Alpine 14.2.0) 14.2.0, 64-bit
[PIPE DEBUG] 2025-03-05 11:18:02.254 UTC [32] LOG:  listening on IPv4 address "0.0.0.0", port 5432
2025-03-05 11:18:02.254 UTC [32] LOG:  listening on IPv6 address "::", port 5432
[PIPE DEBUG] 2025-03-05 11:18:02.256 UTC [32] LOG:  listening on Unix socket "/postgres-volume/run/postgresql/.s.PGSQL.5432"
[PIPE DEBUG] 2025-03-05 11:18:02.259 UTC [35] LOG:  database system was shut down at 2025-03-05 11:18:01 UTC
[PIPE DEBUG] 2025-03-05 11:18:02.262 UTC [32] LOG:  database system is ready to accept connections
[PIPE DEBUG] done
server started
[PIPE DEBUG] [Postgres] Checking PostgreSQL status...
[PIPE DEBUG] pg_ctl: server is running (PID: 32)
/usr/libexec/postgresql17/postgres "-D" "/postgres-volume/run/postgresql/data"
[PIPE DEBUG] [Postgres] Creating database, user and schema...

At this point, PostgreSQL is fully running and accepting connections, yet Execa does not return control to the parent script. The process remains blocked indefinitely instead of completing execution.

Reproduction

Minimal reproduction repository:

Steps to reproduce:

docker build -t pg-blocking-test .
docker run --rm pg-blocking-test

Expected Behavior

  • PostgreSQL initializes and starts
  • Execa returns control after PostgreSQL starts

Actual behavior:

  • PostgreSQL initializes and logs that it is ready to accept connections
  • Execa never returns control to the parent process

Environment

Execa version: 9.5.2
Node.js version: 22.14.0

We've tried modifying stdio, handling streams manually, and experimenting with different ways of spawning the process, but none have resolved the issue. Is this expected behavior, or is there a way to properly detach PostgreSQL so that Execa returns control as expected?

@karlhorky
Copy link
Member

Comment on lines 80 to 83
if (preflightEnvironmentVariables) {
const environmentVariablesKeys: string[] = JSON.parse(
preflightEnvironmentVariables,
);
Copy link
Member Author

Choose a reason for hiding this comment

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

If the environment variables are found, JSON.parse converts the string representation of an array into an array.

'["NEXTAUTH_URL", "APP_SECRET_KEY"]' to ["NEXTAUTH_URL", "APP_SECRET_KEY"]

Comment on lines 76 to 78
const preflightEnvironmentVariables = postgresProcess.stdout.match(
/PREFLIGHT_ENVIRONMENT_VARIABLES:\n(\[.*?\])/,
)?.[1];
Copy link
Member Author

Choose a reason for hiding this comment

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

The alpine-postgresql-setup-and-start script prints PREFLIGHT_ENVIRONMENT_VARIABLES: followed by a string representation of an array. Use .match() to locate this line and extract the array from the next line.

Comment on lines 85 to 88
for (const key of environmentVariablesKeys) {
process.env[key] = `fake_${key.toLowerCase()}`;
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Get a value from the array and create an environment variable with a fake value based on its name. If the array is empty, no additional environment variable will be set

stderr: 'inherit',
})`bash ./scripts/alpine-postgresql-setup-and-start.sh`;

const preflightEnvironmentVariables = postgresProcess.stdout.match(
/PREFLIGHT_ENVIRONMENT_VARIABLES:\n(\[.*?\])/,
Copy link
Member

Choose a reason for hiding this comment

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

match minimum 1 character that is not ] inside of the array, so that we don't do anything for empty arrays

Suggested change
/PREFLIGHT_ENVIRONMENT_VARIABLES:\n(\[.*?\])/,
/PREFLIGHT_ENVIRONMENT_VARIABLES:\n(\[[^\]]+\])/,

);

for (const key of environmentVariablesKeys) {
process.env[key] = `fake_${key.toLowerCase()}`;
Copy link
Member

Choose a reason for hiding this comment

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

  1. maybe 'fake_' isn't expressive enough for a student debugging why they're app isn't working (eg. if their app actually needs the real env var value)

    so probably best to make the prefix or value more expressive

  2. you probably can get rid of the key.toLowerCase() suffix too

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed the fake prefix, and changed it to MISSING_ENV_, so when students debug, they would see MISSING_ENV_CLOUDINARY_API_KEY instead of fake_cloudinary_api_key
5a07f4e

Copy link
Member

Choose a reason for hiding this comment

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

hmm, MISSING_ENV_ is not much more helpful than fake_ - I guess we should be more expressive and make sure students can easily find the source of this (probably should mention Preflight and maybe UpLeveled)

probably also with some kind of mention of "fake" or "dummy" or "placeholder"

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed now to UPLEVELED_PREFLIGHT_PLACEHOLDER, I think students will understand that this comes from Upleveled and is for Preflight as a placeholder.
7dcffd4

@upleveled upleveled deleted a comment from ProchaLu Mar 6, 2025
@upleveled upleveled deleted a comment from ProchaLu Mar 6, 2025
@upleveled upleveled deleted a comment from ProchaLu Mar 6, 2025
@upleveled upleveled deleted a comment from ProchaLu Mar 6, 2025
@upleveled upleveled deleted a comment from ProchaLu Mar 6, 2025
Comment on lines +71 to +72
stdout: ['inherit', 'pipe'],
stderr: ['inherit', 'pipe'],
Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to ['inherit', 'pipe'] for programmatic + terminal output. Print directly in the terminal with inherit and capture the process output with pipe.

Copy link
Member

@karlhorky karlhorky left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Comment on lines +75 to +77
const preflightEnvironmentVariables = postgresProcess.stdout.match(
/PREFLIGHT_ENVIRONMENT_VARIABLES:\n(\[[^\]]+\])/,
)?.[1];
Copy link
Member Author

Choose a reason for hiding this comment

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

The alpine-postgresql-setup-and-start script prints PREFLIGHT_ENVIRONMENT_VARIABLES: followed by a string representation of an array. Use .match() to locate this line and extract the array from the next line.

Comment on lines 79 to 83
if (preflightEnvironmentVariables) {
for (const key of JSON.parse(preflightEnvironmentVariables) as string[]) {
process.env[key] = 'UPLEVELED_PREFLIGHT_PLACEHOLDER';
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

If the script uses additional environment variables, this gets a value from the array and create an environment variable with a UPLEVELED_PREFLIGHT_PLACEHOLDER value. If the array is empty, no additional environment variable will be set

@ProchaLu ProchaLu changed the title Fix Preflight test fails due to missing env variables Prevent Preflight test failures from missing environment variables Mar 6, 2025
@ProchaLu ProchaLu changed the title Prevent Preflight test failures from missing environment variables Add support for student project environment variables Mar 6, 2025
@ProchaLu ProchaLu merged commit bb9ccec into main Mar 6, 2025
6 checks passed
@ProchaLu ProchaLu deleted the add-env-from-example branch March 6, 2025 14:13
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.

None yet

2 participants