CartoDB / Windshaft-cartodb

Windshaft tailored for CARTO
BSD 3-Clause "New" or "Revised" License
72 stars 58 forks source link

Improve project usability #1145

Closed dgaubert closed 4 years ago

dgaubert commented 5 years ago

This is the kind of PR that makes me happier, a.k.a developer happiness:

Algunenano commented 4 years ago

I get the following error when running npm test:

{ Error: Command failed: psql -c "CREATE EXTENSION IF NOT EXISTS cartodb CASCADE;" test_windshaft_cartodb_user_1_db
could not find a "psql" to execute
could not find a "psql" to execute
psql: fatal: could not find own program executable

    at ChildProcess.exithandler (child_process.js:294:12)
    at ChildProcess.emit (events.js:198:13)
    at maybeClose (internal/child_process.js:982:16)
    at Process.ChildProcess._handle.onexit (internal/child_process.js:259:5)
  killed: false,
  code: 1,
  signal: null,
  cmd:
   'psql -c "CREATE EXTENSION IF NOT EXISTS cartodb CASCADE;" test_windshaft_cartodb_user_1_db',
  stdout: '',
  stderr:
   'could not find a "psql" to execute\ncould not find a "psql" to execute\npsql: fatal: could not find own program executable\n' }

psql is accesible and in the search path:

$ which psql
/usr/bin/psql

$ psql
psql (12.1)
Type "help" for help.

raul=# 
Algunenano commented 4 years ago

Is test/support/prepare_db.sh still needed after these changes?

dgaubert commented 4 years ago

Is test/support/prepare_db.sh still needed after these changes?

Nope

Algunenano commented 4 years ago

About the psql thing. It works if I remove the ENV options, so it seems node might be overwritting all environment variables when that's passed. I'd suggest either extending the current env or dropping that and ask PGUSER to be setup properly with superadmin rights.

dgaubert commented 4 years ago

Yes, totally. Going to find a better way to do that.

dgaubert commented 4 years ago

Can you try this, please? https://github.com/CartoDB/Windshaft-cartodb/pull/1146

Algunenano commented 4 years ago

Can you try this, please? #1146

That works for me. Now it runs the tests but I get a different issue, which seems unrelated to this.

dgaubert commented 4 years ago

But I get a different issue, which seems unrelated to this.

Please, let me know if it's related.

Algunenano commented 4 years ago

Looks like the setup used to add cartodb to the search_path of the user and it no longer does it, so I have several failures:

I can create PRs if you want to fix those issues there instead of forcing cartodb to be in the search_path if you want.

dgaubert commented 4 years ago

Whoa!

I think I should keep the previous behavior and set the search_path up. Wondering why you have so many issues with this PR.

dgaubert commented 4 years ago

I thought we were setting search_path here: https://github.com/CartoDB/Windshaft-cartodb/blob/master/test/support/sql/windshaft.test.sql#L14

Algunenano commented 4 years ago

I think I should keep the previous behavior and set the search_path up. Wondering why you have so many issues with this PR.

Probably because things are tested with 3 different flavours of Ubuntu with the same setup.

I thought we were setting search_path here: https://github.com/CartoDB/Windshaft-cartodb/blob/master/test/support/sql/windshaft.test.sql#L14

That doesn't include other users. The failing tests are using test_windshaft_publicuser and test_windshaft_regular1.

dgaubert commented 4 years ago

I see, sorry if I didn't get what you meant: what does this PR different for having so many issues with your setup?

Algunenano commented 4 years ago

I see, sorry if I didn't get what you meant: what does this PR different for having so many issues with your setup?

I don't think this PR has many issues with my setup, instead I'd say that testing with a different setup has uncovered 2 bugs that were either introduced or uncovered by this PR. Ideally those should have been detected by the CI, not me, but we can't test everything.

dgaubert commented 4 years ago

You man, the wild CI.

dgaubert commented 4 years ago

In fact, by solving the issue you had I've realized I can simplify npm-scripts to be able to run coverage.