Skip to content

fix: fail !env substitution when the named variable is missing (fixes #63) - #67

Open
cpruijsen wants to merge 1 commit into
powersync-ja:mainfrom
cpruijsen:fix/issue-63
Open

cpruijsen wants to merge 1 commit into
powersync-ja:mainfrom
cpruijsen:fix/issue-63

Conversation

@cpruijsen

Copy link
Copy Markdown

Summary

!env already recorded a parse error that named a missing variable, but parseYamlFile ignored doc.errors and returned the variable name as the value. validate / deploy then treated uri: PS_DATABASE_URI as a connection string, and Cloud reported PSYNC_S1109 (protocol must be postgresql, got undefined). Parse now fails with that existing message so the missing variable is named, and --validate-only=connections no longer swallows the failure.

The reporter reproduced the Cloud protocol error on #63. Docs and README only describe process environment variables, not .env files.

Decision

  • Chose: Surface the existing !env parse error (name the missing variable). Do not auto-load .env / .env.local.
  • Alternative: Load .env then .env.local from the working directory (without overriding exported variables).
  • Why: The misleading error is a dropped parse failure, not missing dotenv. Auto-loading files is a new lookup policy this repo has never documented.
  • Happy to switch to dotenv loading if that is preferred.

ensureServiceTypeMatches only reads _type, so it uses parseYamlDocumentPreserveTags and does not require template !env placeholders to be set (same idea as the docker plugin when merging snippets).

Testing

  • Unit: parseYamlFile throws naming PS_DATABASE_URI when unset, and still substitutes when set.
  • powersync validate --validate-only=connections fails with that name and does not call testConnection.
  • powersync link self-hosted still works when service.yaml has unresolved !env URIs.

Closes #63

@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@LucDeCaf LucDeCaf left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution! Tested this on my side and it looks good, just a few small observations.
Also, don't forget to sign the CLA. :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I noticed that when running powersync validate and powersync pull instance in the examples/cloud/basic-cloud-pull example, I got two differently formatted errors. This is preexisting and happens because only some calls to parseYamlFile are wrapped in try { ... } catch (error) { this.styledError(...); }. I'll make a followup PR to fix this; just mentioning it here.

Image


/**
* Parses a YAML document, evaluating !env tags.
* Throws when substitution fails (missing or invalid env vars) so callers cannot

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This description is slightly misleading since the function now throws on any YAML parsing error, not just missing env vars.

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.

!env only resolves from exported variables; .env / .env.local are ignored and the error hides the cause

3 participants