Skip to content

fix: read ENVIO_PG_PASSWORD with fallback to ENVIO_POSTGRES_PASSWORD#1098

Open
akandic47 wants to merge 9 commits intoenviodev:mainfrom
akandic47:fix/envio-pg-password-env-var
Open

fix: read ENVIO_PG_PASSWORD with fallback to ENVIO_POSTGRES_PASSWORD#1098
akandic47 wants to merge 9 commits intoenviodev:mainfrom
akandic47:fix/envio-pg-password-env-var

Conversation

@akandic47
Copy link
Copy Markdown

@akandic47 akandic47 commented Apr 10, 2026

Summary

  • Fixes the env var naming inconsistency in packages/cli/src/persisted_state/db.rs
  • Now reads ENVIO_PG_PASSWORD first (matching docs, Env.res, and every other ENVIO_PG_* variable), with a fallback to ENVIO_POSTGRES_PASSWORD for backwards compatibility

Problem

get_pg_pool() was reading ENVIO_POSTGRES_PASSWORD while all other variables use the ENVIO_PG_* convention. This caused envio local db-migrate up to fail with "password authentication failed" when only ENVIO_PG_PASSWORD was set.

Variable Before After
Host ENVIO_PG_HOST ENVIO_PG_HOST
Port ENVIO_PG_PORT ENVIO_PG_PORT
User ENVIO_PG_USER ENVIO_PG_USER
Password ENVIO_POSTGRES_PASSWORD ENVIO_PG_PASSWORD -> ENVIO_POSTGRES_PASSWORD
Database ENVIO_PG_DATABASE ENVIO_PG_DATABASE

Fixes #1097

Summary by CodeRabbit

  • Improvements
    • PostgreSQL connection now checks an additional environment variable for the database password, falls back to the prior variable name, and ultimately uses a safe default when unset—improving compatibility and resilience.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6056832e-0e31-4e7f-bdfe-e705ee062816

📥 Commits

Reviewing files that changed from the base of the PR and between 49e964d and d04dd70.

📒 Files selected for processing (1)
  • packages/cli/src/persisted_state/db.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/cli/src/persisted_state/db.rs

📝 Walkthrough

Walkthrough

Replaced direct environment lookup for the Postgres password in packages/cli/src/persisted_state/db.rs with a small helper and changed password resolution to prefer ENVIO_PG_PASSWORD, falling back to ENVIO_POSTGRES_PASSWORD, and finally "testing" if neither is set.

Changes

Cohort / File(s) Summary
PostgreSQL password resolution
packages/cli/src/persisted_state/db.rs
Added a small env helper (get_env_with_default); changed Postgres pool/password sourcing to read ENVIO_PG_PASSWORDENVIO_POSTGRES_PASSWORD"testing" default. (No test changes in this diff.)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Suggested reviewers

  • JonoPrest

Poem

🐰 I hop through envs with a curious twitch,
Preferring ENVIO_PG_ before the old switch.
If both are quiet, "testing" I keep,
A tidy little secret, soft and neat.
Hooray for consistent hops and leaps! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: reading ENVIO_PG_PASSWORD with fallback to ENVIO_POSTGRES_PASSWORD, which is the core fix in the changeset.
Linked Issues check ✅ Passed The PR implementation fully addresses all objectives from issue #1097: reading ENVIO_PG_PASSWORD first with ENVIO_POSTGRES_PASSWORD fallback, maintaining backwards compatibility, and fixing authentication failures.
Out of Scope Changes check ✅ Passed The changes are narrowly scoped to address the password environment variable issue; no unrelated modifications to test files or other areas are present in the diff.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
packages/cli/src/persisted_state/db.rs (1)

20-20: Please add a focused regression test for env var precedence.

Line 20 changes runtime config behavior; a small unit test for get_env_with_fallback (primary set, fallback-only set, neither set) would lock this in and prevent future regressions.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@packages/cli/src/persisted_state/db.rs` at line 20, Add a focused unit test
for get_env_with_fallback covering three scenarios: (1) primary env var set
(ENVIO_PG_PASSWORD) and fallback also set — assert primary wins; (2) only
fallback env var set (ENVIO_POSTGRES_PASSWORD) — assert fallback is returned;
(3) neither set — assert default "testing" is returned. Use a test that sets and
unsets the environment variables (std::env::set_var/remove_var) around each case
and call get_env_with_fallback to assert expected results; place the test in the
same module or a tests mod so it runs with cargo test to prevent regressions in
the runtime config precedence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@packages/cli/src/persisted_state/db.rs`:
- Line 20: Add a focused unit test for get_env_with_fallback covering three
scenarios: (1) primary env var set (ENVIO_PG_PASSWORD) and fallback also set —
assert primary wins; (2) only fallback env var set (ENVIO_POSTGRES_PASSWORD) —
assert fallback is returned; (3) neither set — assert default "testing" is
returned. Use a test that sets and unsets the environment variables
(std::env::set_var/remove_var) around each case and call get_env_with_fallback
to assert expected results; place the test in the same module or a tests mod so
it runs with cargo test to prevent regressions in the runtime config precedence.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e4a9c711-0422-4191-8603-446b299a86cd

📥 Commits

Reviewing files that changed from the base of the PR and between c642ec4 and ae49460.

📒 Files selected for processing (1)
  • packages/cli/src/persisted_state/db.rs

@akandic47 akandic47 force-pushed the fix/envio-pg-password-env-var branch from ae49460 to 49e964d Compare April 10, 2026 16:57
The persisted_state db.rs was reading ENVIO_POSTGRES_PASSWORD while every
other Postgres env var follows the ENVIO_PG_* convention. This caused
db-migrate up to fail with auth errors when only ENVIO_PG_PASSWORD was set.

Now reads ENVIO_PG_PASSWORD first (matching docs and Env.res), falling back
to ENVIO_POSTGRES_PASSWORD for backwards compatibility.

Fixes enviodev#1097
@akandic47 akandic47 force-pushed the fix/envio-pg-password-env-var branch from 49e964d to d04dd70 Compare April 10, 2026 17:03
Copy link
Copy Markdown
Member

@DZakh DZakh left a comment

Choose a reason for hiding this comment

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

Nice find 👍

@DZakh
Copy link
Copy Markdown
Member

DZakh commented Apr 11, 2026

Looks like CI fails because it doesn't get access to secrets. I'll investigate how we can solve it for internal contributors on Tuesday when I'm back from my vacation.

@akandic47
Copy link
Copy Markdown
Author

Nice find 👍

Thnx... We had some problems with propagating migrations so decided to investigate further and came up to this :D

@DZakh
Copy link
Copy Markdown
Member

DZakh commented Apr 14, 2026

/ok-to-test baeef6d

@DZakh
Copy link
Copy Markdown
Member

DZakh commented Apr 14, 2026

/ok-to-test baeef6d

@akandic47 Please ignore the noise. I'm trying to bypass the CI secrets limitation while finishing other changes for the next release. I'll make sure it's merged and included to the next release anyways.

@DZakh
Copy link
Copy Markdown
Member

DZakh commented Apr 15, 2026

/ok-to-test fe3e955

@DZakh
Copy link
Copy Markdown
Member

DZakh commented Apr 15, 2026

/ok-to-test 047643d

@DZakh
Copy link
Copy Markdown
Member

DZakh commented Apr 15, 2026

/ok-to-test 81d8e0b

DZakh pushed a commit that referenced this pull request Apr 16, 2026
Add `pending-status` job: on `pull_request` events from forks, posts a
`build-verify: pending` commit status to the fork PR's head SHA.

This solves two problems observed on PR #1098:

1. Fork PR had all checks "Skipped" (because the gate blocks forks on
   `pull_request` events). GitHub branch protection treats "Skipped" as
   passing → PR was mergeable without any CI running. With the pending
   status, branch protection (configured to require `build-verify`)
   sees "pending" → blocks merge.

2. After /ok-to-test triggered CI via `issue_comment`, the native check
   runs attached to the default-branch SHA (invisible on the PR). The
   `report` job already posts `build-verify: success/failure` to the
   fork PR's head SHA — this overrides the pending status, and is the
   only status branch protection needs to require.

Each new fork push re-triggers `pull_request` → re-posts pending →
re-blocks merge until a reviewer re-approves via /ok-to-test.

After merging, configure branch protection:
  Settings → Branches → main → Require status checks →
  add `build-verify` as a required context.
DZakh pushed a commit that referenced this pull request Apr 16, 2026
Add `pending-status` job: on `pull_request` events from forks, posts a
`build-verify: pending` commit status to the fork PR's head SHA.

This solves two problems observed on PR #1098:

1. Fork PR had all checks "Skipped" (because the gate blocks forks on
   `pull_request` events). GitHub branch protection treats "Skipped" as
   passing → PR was mergeable without any CI running. With the pending
   status, branch protection (configured to require `build-verify`)
   sees "pending" → blocks merge.

2. After /ok-to-test triggered CI via `issue_comment`, the native check
   runs attached to the default-branch SHA (invisible on the PR). The
   `report` job already posts `build-verify: success/failure` to the
   fork PR's head SHA — this overrides the pending status, and is the
   only status branch protection needs to require.

Each new fork push re-triggers `pull_request` → re-posts pending →
re-blocks merge until a reviewer re-approves via /ok-to-test.

After merging, configure branch protection:
  Settings → Branches → main → Require status checks →
  add `build-verify` as a required context.
@DZakh DZakh deployed to fork-ci April 16, 2026 12:10 — with GitHub Actions Active
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.

bug: Rust CLI uses ENVIO_POSTGRES_PASSWORD instead of ENVIO_PG_PASSWORD

2 participants