feat: add root option to afterReset hooks of type command

jcgsville commented 3 years ago

Description

Fix for issue 132. afterReset actions of type command now respect the root property in the action spec

Performance impact

I can't imagine a performance cost to this

Security impact

None that I'm aware of

Checklist

[x] My code matches the project's code style and yarn lint:fix passes.
[x] I've added tests for the new feature, and yarn test passes.
[x] I have detailed the new feature in the relevant documentation.
[ ] I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
[x] If this is a breaking change I've explained why.

I wasn't able to get my unit tests to run locally. Happy to try to write a test for this if you have any advice on what I'm missing? This was the behavior when I checked out the commit before mine so none of my changes are involved here.

yarn test
yarn run v1.22.10
$ yarn lint && depcheck --ignores @types/jest,eslint_d,tslib && yarn run test:only --ci
$ yarn prettier:check && eslint --ext .js,.jsx,.ts,.tsx,.graphql .
$ prettier --ignore-path .eslintignore --check '**/*.{js,jsx,ts,tsx,graphql,md,json}'
Checking formatting...
docs/docker/README.md
docs/idempotent-examples.md
README.md
Code style issues found in the above file(s). Forgot to run Prettier?
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Also, I don't see a RELEASE_NOTES.md, so I made no additions there.

benjie commented 3 years ago

Huh! Looks like we weren't running prettier in CI?! Fixed in #138 - please rebase, then hopefully you can run the tests :)

jcgsville commented 2 years ago

@benjie I think this PR is ready to go with the exception of the release notes checkbox. Not sure if I'm missing where to add those?

What's changed since your 👀 were on this:

I added more detail to the readme to make clear the behavior of the root property for command actions.
I modified GM_DBUSER to be undefined when root = true at your suggestion to avoid confusion. I also noted this in the readme.
I added a unit test testing that afterReset command actions with root: true behave as intended
I added a bonus unit test testing that afterReset SQL actions with root: true behave as intended

Apologies that I didn't realize force pushing to my branch would break Github's requested changes. The change you requested was a small tweak to the README which I implemented.

Finally, as I was working on the unit tests, I found a separate bug in makeRootDatabaseConnectionString(). This should probably be a separate PR to fix this. If it makes more since for it to be one, let me know.

The 🐛 : when setting rootConnectionString to just the db name, the connection string used when root: true incorrectly had a slash prepended to the database name.

For example, when rootConnectionString: "postgres", connectionString: "postgres://localhost/app_db" the connection string when root: true should be postgres but it was /postgres.

I noticed this because the unit tests default rootConnection string was just postgres and the assertions were failing unexpectedly. I've modified settings.ts:420 to fix it, and I believe this change produces the desired behavior. But it's worth your 👀 to make sure I didn't misunderstand something. I also added a unit test to cover this case. I believe the existing unit tests on this function were sufficient to prove that this change introduced no regressions.

jcgsville commented 2 years ago

@benjie know of anything left before this can be merged?

jcgsville commented 2 years ago

I addressed the recommendations to fix the tests. Last thing pending is resolution surrounding the pathname vs dbname debacle

benjie commented 2 years ago

My new test didn't pass linting :(

benjie commented 2 years ago

I tried fixing it using prettier playground but that wasn't enough; please reformat :+1:

jcgsville commented 2 years ago

Thanks for your thorough reviews!

benjie commented 2 years ago

Of course it was the trailing comma :man_facepalming:

benjie commented 2 years ago

Released in 1.3.0 :raised_hands:

graphile / migrate