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

how to use with graphile-worker #178

Open
ssendev opened this issue Jan 26, 2023 · 4 comments
Open

how to use with graphile-worker #178

ssendev opened this issue Jan 26, 2023 · 4 comments

Comments

@ssendev
Copy link

ssendev commented Jan 26, 2023

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.

@benjie
Copy link
Member

benjie commented Jan 26, 2023

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.

@benjie benjie transferred this issue from graphile/worker Jan 26, 2023
@benjie benjie changed the title how to use with graphile-migrate how to use with graphile-worker Jan 26, 2023
@benjie
Copy link
Member

benjie commented Jan 26, 2023

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

@ssendev
Copy link
Author

ssendev commented Jan 26, 2023

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.

@benjie
Copy link
Member

benjie commented Jan 26, 2023

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.

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

No branches or pull requests

2 participants