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

Add Aurora Postgres Connections #425

Merged
merged 62 commits into from
Sep 13, 2021
Merged

Add Aurora Postgres Connections #425

merged 62 commits into from
Sep 13, 2021

Conversation

mojotalantikite
Copy link
Contributor

@mojotalantikite mojotalantikite commented Sep 9, 2021

Summary

This change brings in the configurations that I tested out in #375 and #351 in a clean branch. I wanted serverless to attempt a new deploy via a new stage after all the testing that happened in other branches.

It adds:

  • Prisma bundled up with webpack in order to generate schemas and establish DB connections
  • A handler which we can test the postgres connection. It pulls the DB secrets from secrets manager, connects to aurora via Prisma, and executes a test query that pulls basic analytics from the cluster.

Related issues

https://qmacbis.atlassian.net/browse/OY2-10859

@mojotalantikite mojotalantikite changed the title Add Aurora Postgres Add Aurora Postgres Connections Sep 9, 2021
Copy link
Contributor

@macrael macrael left a comment

Choose a reason for hiding this comment

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

Looks great, I'm so excited for this.

}

// A model is required for prisma. Dummy model for our testing.
model TestUser {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this table exist in our postgres db? Where did that get created?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't yet. Prisma refuses to generate a schema without any models and I was just trying to get Prisma client working along with bundling it up into the lambda. So this is just me adding a dummy model to get Prisma to run. I was going to do the migrations work in this epic.

@@ -0,0 +1,73 @@
const CopyWebpackPlugin = require('copy-webpack-plugin');
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to use require() here? Everywhere else on the project we're doing:

import * as CopyWebpackPlugin from 'copy-webpack-plugin'

Copy link
Contributor

Choose a reason for hiding this comment

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

or import CopyWebpackPlugin from 'copy-webpack-plugin, whichever works.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually hitting a bunch of weirdness with webpack when trying to use import statements. Here's the error when you switch to imports:

 Syntax Error --------------------------------------------

  /Users/mojo/clients/managedcare/managed-care-review/services/app-api/webpack.config.js:1
  import CopyWebpackPlugin from 'copy-webpack-plugin';

  SyntaxError: Cannot use import statement outside a module

And then when you move package.json to define as a module you get:

 Error ---------------------------------------------------

  Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/mojo/clients/managedcare/managed-care-review/services/app-api/webpack.config.js
  require() of ES modules is not supported.
  require() of /Users/mojo/clients/managedcare/managed-care-review/services/app-api/webpack.config.js from /Users/mojo/clients/managedcare/managed-care-review/services/app-api/node_modules/serverless-webpack/lib/validate.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "m
odule" which defines all .js files in that package scope as ES modules.
  Instead rename webpack.config.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/mojo/clients/managedcare/managed-care-review/services/app-api/package.json.

I'm sure there's a fix I just don't know about. I can try to fix that up.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't waste too much time on it, this is a config file

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you simply can't use es6 import syntax in a file also using module.exports format. Its a node thing - the way this file is written is called (I think) commonjs format - and its different than es6 modules like is used in a our api and web code that we build.

)
}

export const installPrismaDepsOnce = once(installPrismaDeps)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nothing wrong with this, but probably unnecessary, I don't think we ever call installAPIDeps twice in the same script. (we do with installWebDeps when you run web and storybook)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I was just trying to follow the example of how we're doing graphql things, but if you think it's not needed I'm happy to drop using once.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, with graphQL we need once b/c it's started by both web and api but this is only run by api

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, I removed the once wrappers here then.

includeModules: true
excludeRegex: 'darwin|debian'
packagerOptions:
noFrozenLockfile: true
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 wanted to highlight this. Webpack kept throwing errors unless noFrozenLockfile was set to true for us. This might impact reproducibility of our dependencies, so figured we should talk about it.

Copy link
Contributor

Choose a reason for hiding this comment

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

oh weird, so it's possible for web pack to update our lockfile?

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like this is fine. We are still figuring all of this out.

I am curious though what you @mojotalantikite tried to address the issue (roughly). Should we backlog a ticket to revisit this briefly when we finish the migration - I can take that work on if I can get the ctkey stuff working. From what I can tell, we should be able to install with a frozen lockfile (was reading prisma docs and the serverless-webpack docs) so It could be something super minor about our package.json or the webpack conifg being still not quite right.

re: macrae pretty sure any time you run yarn, which webpack is also doing for us to bundle stuff, your lockfile can change randomly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@haworku I tried re-generating the lockfile a couple times as well as trying to see if there were deps that we were using that weren't playing well with webpack. There is a thread here I was reading a couple days ago and it likely is something to do with the dependencies not being in the right spot.

I'll give them another look over to see if I can spot it. At the time I just set this as true out of frustration so I could keep it moving.

Copy link
Contributor

@haworku haworku Sep 10, 2021

Choose a reason for hiding this comment

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

This shouldn't block our work here. Just was curious. No need to continue on the pain now just wondering if we should do as a cleanup thing. Yeah could be something that its devDependencies should actually be in dependencies ugh

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@haworku I just figured it out. I re-ran yarn add at the point release for all the tools that get packaged up with the nodeExternals() function in webpack and for whatever reason @aws/dynamodb-data-mapper is a dependency that was not in our package.json. I'm not sure where that got dropped or if it just was a dependency that was pulled in via other dependencies and existed in our lock file, but it looks like webpack is packaging it properly now. I dropped the noFrozenLockfiles piece right now and am waiting to see if it will pass CI.

securityGroupIds:
- ${self:custom.sgId}
subnetIds: ${self:custom.privateSubnets}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to highlight this as well. In order to connect to Aurora, the lambda needs to be in the same VPC. Aurora has another sort of endpoint that doesn't require the lambda to be in the VPC, but Prisma doesn't support it at this point.

services/app-api/package.json Show resolved Hide resolved
}

// A model is required for prisma. Dummy model for our testing.
model TestUser {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't yet. Prisma refuses to generate a schema without any models and I was just trying to get Prisma client working along with bundling it up into the lambda. So this is just me adding a dummy model to get Prisma to run. I was going to do the migrations work in this epic.

@@ -0,0 +1,73 @@
const CopyWebpackPlugin = require('copy-webpack-plugin');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually hitting a bunch of weirdness with webpack when trying to use import statements. Here's the error when you switch to imports:

 Syntax Error --------------------------------------------

  /Users/mojo/clients/managedcare/managed-care-review/services/app-api/webpack.config.js:1
  import CopyWebpackPlugin from 'copy-webpack-plugin';

  SyntaxError: Cannot use import statement outside a module

And then when you move package.json to define as a module you get:

 Error ---------------------------------------------------

  Error [ERR_REQUIRE_ESM]: Must use import to load ES Module: /Users/mojo/clients/managedcare/managed-care-review/services/app-api/webpack.config.js
  require() of ES modules is not supported.
  require() of /Users/mojo/clients/managedcare/managed-care-review/services/app-api/webpack.config.js from /Users/mojo/clients/managedcare/managed-care-review/services/app-api/node_modules/serverless-webpack/lib/validate.js is an ES module file as it is a .js file whose nearest parent package.json contains "type": "m
odule" which defines all .js files in that package scope as ES modules.
  Instead rename webpack.config.js to end in .cjs, change the requiring code to use import(), or remove "type": "module" from /Users/mojo/clients/managedcare/managed-care-review/services/app-api/package.json.

I'm sure there's a fix I just don't know about. I can try to fix that up.

)
}

export const installPrismaDepsOnce = once(installPrismaDeps)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, I was just trying to follow the example of how we're doing graphql things, but if you think it's not needed I'm happy to drop using once.

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

Successfully merging this pull request may close these issues.

3 participants