Disable default mutations is now in config file #89

ana0 · 2019-04-29T21:19:44Z

Do folks think it should be an env var too?

m0ar · 2019-04-30T11:28:13Z

@ana0 Hey! Personally can't find a reason to have GQL mutations at all for vDB.

This should indeed be the default, but I don't think we need this embedded postgraphile code any more. AFAIK the before-it-was-open-source subscription stuff was the only reason for using postgraphile as a library, so I think we should be able to use the postgraphile binary externally and use ye olde' CLI flags to configure everything we need 🤷‍♀️ That'd mean the only connection between vDB and postgraphile would be some README material on how to configure it (which would make @AFDudley happy 💯 )

I haven't found the time to try it our properly yet, but I think @elizabethengelman has done some tests on this?

ana0 · 2019-04-30T12:11:06Z

Veriledger needs mutations in order to add new addresses and contracts to the account transformer (just for the accounts schema), and are currently building off this branch. But if the preferred path forward is to remove the embedded postgraphile and have them build with the binary directly, that should be fine too. Someone will need to test it out and provide support to them while they set it up.

m0ar · 2019-04-30T12:18:53Z

@ana0 Ah, that's cool. Then definitely go ahead with this for now. When we phase out postgraphile from this codebase it'll be easier for everyone, instead of having to build it from source etc. AFAIK one would just invokes the vanilla postgraphile binary and pass some configuration flags we would list in the README.

m0ar

🚀

m0ar · 2019-04-30T12:21:10Z

postgraphile/src/config/parse.ts

@@ -37,6 +38,9 @@ export function parseConfig(
    gqSchemas = parsedToml['database']['gq_schemas'];
    gqUser = parsedToml['database']['gq_user'] || gqUser;
    gqPassword = parsedToml['database']['gq_password'] || gqPassword;
+    disableDefaultMutations = parsedToml['database']['disable_default_mutations'] === undefined


I'm no TS/JS wizard, what's the difference between this and the disjunctions above? I think this would behave unexpectedly on an empty string for example.

Do you mean if there were an empty string in the config.toml? In that case, it'll ignore the incorrect var and keep the default true. I did it this way because there are expected cases where disableDefaultMutations will be falsey.

So, if disable_default_mutations = false in the config.toml, disableDefaultMutations = parsedToml['database']['disable_default_mutations'] || true; would still end up true.

Ah, makes sense 👌

i-norden

LGTM! I just have a single comment, not blocking.

I am definitely all for removing Postgraphile entirely from this codebase in the future, if possible.

i-norden · 2019-04-30T16:34:04Z

postgraphile/README.md

@@ -23,8 +23,7 @@ Build the docker image in this directory. Start the `GraphiQL` frontend by:
 * GraphiQL frontend is available at `:3000/graphiql`
  GraphQL endpoint is available at `:3000/graphql`

-By default, this build will expose only the "public" schema - to add other schemas, use either the env variables,
-or a config file `config.toml` and set the env var `CONFIG_PATH` to point to its location. Example `toml`:
+By default, this build will expose only the "public" schema and will disable mutations - to change mutation behaviour, you can use an optional config file `config.toml` and set the env var `CONFIG_PATH` to point to its location. Example `toml`:


This isn't specific to this PR since CONFIG_PATH was introduced earlier, but I wonder if we should use a more specific env variable name for this? How "hard-coded" is this in Postgraphile? Seems unlikely but CONFIG_PATH is pretty generic and could potentially run into a conflict.

* Use transformer factory for Bite - introduces separate transformer factory for non-LogNote events - converter includes `ToEntities` for events defined via ABI * Updates after rebasing with staging

rmulhol

This looks good to me. I am partial to Ian's comment about potentially changing the name of the environment variable if we can, but not sure if that's out of scope. Also wanted to check in on whether #90 serves our use cases - feel like it might obsolete this PR but also don't want to fire away if it turns out that folks won't be able to run postgraphile how they want without the bespoke code.

ana0 · 2019-05-02T14:05:33Z

I just pushed to change CONFIG_PATH to POSTGRAPHILE_CONFIG_PATH - and yes, agree that #90 is likely to obsolete this. Might be best to co-ordinate with the Veriledger team and see how they do with the stock postgraphile before closing?

m0ar · 2019-05-03T09:09:22Z

@ana0 For reference, this seems to do the trick for us (sans --owner-connection-string until we/graphile builds a postgraphile@4.4.0 docker image):

docker run --network="host" graphile/postgraphile --connection postgres://herp:derp@localhost:5432/vulcanize_public --schema maker --disable-default-mutations --no-ignore-rbac --watch --owner-connection-string postgres://bleep:bloop@localhost:5432/vulcanize_public

Should be equivalent to the bespoke setup, modulo the pg-simplify-inflector plugin which can be added with --append-plugins if it's installed

Disable default mutations is now in config file

bc3f0d5

ana0 requested review from m0ar, elizabethengelman, Gslaughl, i-norden, rmulhol and yaoandrew April 29, 2019 21:37

m0ar approved these changes Apr 30, 2019

View reviewed changes

i-norden approved these changes Apr 30, 2019

View reviewed changes

rmulhol approved these changes May 1, 2019

View reviewed changes

Config path name change

754c8cb

ana0 merged commit 202fb6e into staging May 2, 2019

ana0 deleted the feature/configurableMutations branch May 2, 2019 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable default mutations is now in config file #89

Disable default mutations is now in config file #89

ana0 commented Apr 29, 2019

m0ar commented Apr 30, 2019 •

edited

Loading

ana0 commented Apr 30, 2019 •

edited

Loading

m0ar commented Apr 30, 2019

m0ar left a comment

m0ar Apr 30, 2019

ana0 Apr 30, 2019

m0ar May 1, 2019

i-norden left a comment

i-norden Apr 30, 2019

rmulhol left a comment

ana0 commented May 2, 2019

m0ar commented May 3, 2019 •

edited

Loading

Disable default mutations is now in config file #89

Disable default mutations is now in config file #89

Conversation

ana0 commented Apr 29, 2019

m0ar commented Apr 30, 2019 • edited Loading

ana0 commented Apr 30, 2019 • edited Loading

m0ar commented Apr 30, 2019

m0ar left a comment

Choose a reason for hiding this comment

m0ar Apr 30, 2019

Choose a reason for hiding this comment

ana0 Apr 30, 2019

Choose a reason for hiding this comment

m0ar May 1, 2019

Choose a reason for hiding this comment

i-norden left a comment

Choose a reason for hiding this comment

i-norden Apr 30, 2019

Choose a reason for hiding this comment

rmulhol left a comment

Choose a reason for hiding this comment

ana0 commented May 2, 2019

m0ar commented May 3, 2019 • edited Loading

m0ar commented Apr 30, 2019 •

edited

Loading

ana0 commented Apr 30, 2019 •

edited

Loading

m0ar commented May 3, 2019 •

edited

Loading