graphile/migrate

how to use with graphile-worker

ssendev opened this issue · 4 comments

Summary

I am using graphile-worker and am unsure how to mesh them together

// .gmrc.js
{
  afterReset: [
    {
      _: 'command',
      command:
        'DATABASE_URL="$OWNER_DATABASE_URL" npx --no-install graphile-worker --schema-only',
      shadow: false,
    },
    {
      _: 'command',
      command:
        'DATABASE_URL="$SHADOW_DATABASE_URL" npx --no-install graphile-worker --schema-only',
      shadow: true,
    },
  ],
  ...
}

seems to get the job done but is this future proof or is there a better way?

If for example i have the following migration

create or replace function app_private.add_job(identifier text, payload json DEFAULT NULL::json, queue_name text DEFAULT NULL::text, run_at timestamp with time zone DEFAULT NULL::timestamp with time zone, max_attempts integer DEFAULT NULL::integer, job_key text DEFAULT NULL::text, priority integer DEFAULT NULL::integer, flags text[] DEFAULT NULL::text[], job_key_mode text DEFAULT 'replace'::text) RETURNS graphile_worker.jobs as $$
  select graphile_worker.add_job(identifier, payload, queue_name, run_at, max_attempts, job_key, priority , flags, job_key_mode);
$$ language sql security definer;

and in the future graphile-worker changes add_job or jobs would the migration fail? is there a way around it?

Either way mentioning the way to integrate with graphile-worker in the readme would be nice.

It probably makes sense to add it to beforeAllMigrations too, just to ensure it's always up to date before your migrations take place. I'm not sure about the envvars.

and in the future graphile-migrate changes add_job or jobs would the migration fail? is there a way around it?

Shouldn't do; it's never broken my migrations and we've changed the signature of add_job quite a few times.

I've moved this to the (more relevant) migrate repo.

A yes beforeAllMigrations makes sense but makes afterReset redundant.

For some reason I didn't think to look in the graphle-migrate readme.

Now that I have there is an example with

{
  "afterReset": [
    "afterReset.sql",
    { "_": "command", "command": "npx --no-install graphile-worker --once" }
  ],
}

Which makes the same mistake I had made and uses afterReset it also uses --once instead of --schema-only which was probably caused by having been written before --schema-only existed.

The stuff with the envvars is because $DATABASE_URL is postgres://PLEASE:USE@GM_DBURL/INSTEAD but is probably better solved by using $GM_DBURL which means currently the example should probably be updated to

{
  "afterReset": [
    "afterReset.sql",
  ],
  "beforeAllMigrations": [
    { "_": "command", "command": "npx --no-install graphile-worker --schema-only -c \"$GM_DBURL\"" }
  ],
}

I used the DATABASE_URL="GM_DBURL" npx --no-install graphile-worker --schema-only syntax since it avoids leaking the credentials in the command arguments but has the drawback of not working on windows. Considering it's not running in a shell but probably gets spawned as exec('sh', '-c', command) it would be leaked anyway.

The proper fix would be to eiter let graphile-worker use $GM_DBURL if $DATABASE_URL = postgres://PLEASE:USE@GM_DBURL/INSTEAD or add an --env-connection option where the envvar to use is specified.

Thanks for the quick reply and soothing my worries about future compatibility.

It'd probably make sense to give the _: "command" entries an env dict that can be used to affect the environment. Maybe something like:

{
  "_": "command",
  "command": "npx --no-install graphile-worker --schema-only",
  "env": {
    // Regular envvars, state verbatim
    "SOMETHING": "123456",
    // Source the value for this `DATABASE_URL` envvar from `$GM_DBURL`
    "DATABASE_URL": {"from": "GM_DBURL"}
  }
}

And yes, that afterReset was added before the before options existed, before --schema-only was a thing, and before I (deliberately) broke $DATABASE_URL. So it's overdue for an update - thanks for bringing it to my attention.

Do you fancy taking any of this on? I think the env dict above would be the first step.