-
Notifications
You must be signed in to change notification settings - Fork 0
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
postgres-manager: Initiate postgres-manager #126
Conversation
lib/workload/stateless/postgres_manager/construct/postgresManager.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, Will. Minor comments to attend.
lib/workload/stateless/postgres_manager/function/create-pg-login-role.ts
Outdated
Show resolved
Hide resolved
lib/workload/stateless/postgres_manager/function/create-pg-login-role.ts
Outdated
Show resolved
Hide resolved
{ Key: 'creator', Value: 'postgres_microservice at the orcabus stateless stack' }, | ||
], | ||
}; | ||
const smCommand = new CreateSecretCommand(smInput); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While at it, would you mind set up rotation as well pls. That'd make the set up perfect.
But, if you determine, you'd need more time working on rotation part then pls feel free to tackle as a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, we should rotate the master too..!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes thanks for feedback, I think would be better be part on a seperate PR along with the master secret rotation
lib/workload/stateless/postgres_manager/construct/postgresManager.ts
Outdated
Show resolved
Hide resolved
// 1. lambda responsible on db creation | ||
const createPgDb = new nodejs.NodejsFunction(this, 'CreateDbPostgresLambda', { | ||
...rdsLambdaProps, | ||
entry: __dirname + '/../function/create-pg-db.ts', | ||
functionName: 'orcabus-create-pg-db', | ||
}); | ||
masterSecret.grantRead(createPgDb); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if we could use this kind of pattern for database migration scripts too? I don't know if that would work though, because EdgeDb has it's own migration logic. Although, for anything using postgres directly, we could agree on some common database migration format and re-use a Lambda function like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think it make sense just that it would probably use its own microservice login credential and perhaps the migration lambda would live under its own microservice repo/folder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't you want to keep migrations (and everything that related to application specifics) together with / as part of the application?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's true. I guess I was just thinking in terms of generic logic that applies SQL migrations irrespective of application logic. I.e. receives as input a bunch of .sql
files and applies the migration, recording it in a migration table.
For example, sqlx creates a _sqlx_migrations
table where it has the migration version, checksum, description, success status, etc.
Something like this might be common across applications?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if that's always a common approach across apps, but also you'd split your application concerns across multiple stacks / actors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that's a good point - it's cleanest to leave database table creation to the actual app that is using those tables.
closes #99
This postgres manager will be responsible for
See README for lambda details.
(these lambda should be invoked manually)