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

feat: add support for dropping the database #67

Merged
merged 9 commits into from
Oct 2, 2020

Conversation

alvincrespo
Copy link
Contributor

@alvincrespo alvincrespo commented Oct 1, 2020

This PR ensures that a developer can drop their database from the script db:drop

Changes

  • Adds the script db:drop to a generated apps package.json
  • Includes Mocha for unit testing Bison
  • Creates an createApp script to create the app but NOT start it (to verify setup)
  • Defines a unit test to ensure that DATABASE_NAME is properly generated in prisma/.env
  • Extends our Release workflow to include unit tests
  • Creates a test:prisma script for independently testing prisma setup

Checklist

  • Requires dependency update?
  • Generating a new app works

@alvincrespo alvincrespo added the enhancement New feature or request label Oct 1, 2020
@alvincrespo alvincrespo requested a review from cball October 1, 2020 19:04
@alvincrespo alvincrespo self-assigned this Oct 1, 2020
const execa = require("execa");
module.exports = {};

async function createAppAndStartServer() {
async function init() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just keeping the main method calls in sync across scripts.


fs.writeFile("./tmp/tmpDir", tmpDir, () => {
console.log("tmpDir location stored.");
});
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're storing the tmp directory location so we can pick it up easily if needed.

@@ -11,6 +11,7 @@
"cypress:local": "CYPRESS_LOCAL=true CYPRESS_BASE_URL=http://localhost:3000 cypress",
"db:migrate": "yarn -s prisma migrate up --experimental",
"db:setup": "yarn db:migrate && yarn prisma generate",
"db:drop": "source prisma/.env && dropdb $(echo $DATABASE_NAME)",
Copy link
Contributor Author

@alvincrespo alvincrespo Oct 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dropdb is a command line helper for postgres. Comes with every install - more info here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about putting this into scripts/dropDatabase? If we did that we could do:

import { Client } from 'pg';

const connectionString = process.env.DATABASE_URL;
const db = new Client({ connectionString });
const { database } = db.connectionParameters;

async function drop() {
  await db.connect();
  console.log(`dropping the ${database} database`);
  await db.query(`DROP DATABASE IF EXISTS ${database}`);

  await db.end();
}

drop();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the big advantage here I think is that we can just use the existing DATABASE_URL and not have to manage another variable. we should have pg included as a dependency already.

@alvincrespo
Copy link
Contributor Author

@cball I wasn't sure if we wanted to use Mocha for unit tests - but went down that route to at least get something running. Let me know if you have a library pref and I can update.

Copy link
Member

@cball cball left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome @alvincrespo! It might be good to stick with jest since that's what we are leveraging once the app is generated. Looking at what you have for Mocha I don't think it will be much of an update.

Along the lines of what you mentioned, I'd be curious if our unit tests need to go through the whole app generation process, but we can circle back to that.

The biggest suggestion I have is around creating a script to drop the db. If we go that route, you'll want something like this:

{
  "db:drop": "DOTENV_CONFIG_PATH=./prisma/.env ts-node -r dotenv/config ./scripts/dropdb",
}

@@ -31,6 +31,9 @@ jobs:
- name: Install packages
run: |
yarn install --frozen-lockfile
- name: Run Prisma Unit Tests
run: |
yarn test:prisma
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we make this generic unit tests?

package.json Outdated
@@ -44,6 +45,7 @@
"@semantic-release/git": "^9.0.0",
"cypress": "^5.1.0",
"ejs-lint": "^1.1.0",
"mocha": "^8.1.3",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like I mentioned in the comment I think we should keep this as jest for continuity sake. glad you got this in though!

return path.join(tmpdir, name);
}

init();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@@ -5,3 +5,4 @@
# See the documentation for all the connection string options: https://pris.ly/d/connection-strings

DATABASE_URL="postgresql://<%= db.dev.user %><%= db.dev.password %>@<%= db.dev.host %>:<%= db.dev.port %>/<%= db.dev.name %>?schema=public"
DATABASE_NAME="<%= db.dev.name %>"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we go the scripts route, may not need this

@alvincrespo
Copy link
Contributor Author

@cball Awesome recommendations! This makes sense to me. I'll go ahead and implement.

@alvincrespo
Copy link
Contributor Author

@cball Ready for another round of review.

@alvincrespo alvincrespo merged commit b0d8424 into canary Oct 2, 2020
@alvincrespo alvincrespo deleted the chore/add-support-for-dbdrop branch October 2, 2020 18:09
github-actions bot pushed a commit that referenced this pull request Oct 2, 2020
# [1.7.0-canary.1](v1.6.4...v1.7.0-canary.1) (2020-10-02)

### Features

* add support for dropping the database ([#67](#67)) ([b0d8424](b0d8424))
@github-actions
Copy link

github-actions bot commented Oct 2, 2020

🎉 This PR is included in version 1.7.0-canary.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions bot pushed a commit that referenced this pull request Oct 7, 2020
# [1.7.0](v1.6.4...v1.7.0) (2020-10-07)

### Features

* add support for dropping the database ([#67](#67)) ([b0d8424](b0d8424))
* migrate to @nexus/schema from nexus framework ([#68](#68)) ([4c7d557](4c7d557))
@github-actions
Copy link

github-actions bot commented Oct 7, 2020

🎉 This PR is included in version 1.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Issue has been included in a PR label Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released on @canary released Issue has been included in a PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants