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

359 - One postgres instance for both keycloak and general database #360

Merged
merged 6 commits into from
Nov 17, 2022

Conversation

dimitur2204
Copy link
Contributor

@dimitur2204 dimitur2204 commented Nov 10, 2022

Solves #359

  • Added a script to create a keycloak database on the current postgres instance.
  • Changed the keycloak to now connect to the newly created database

Made the changes that @imilchev proposed in an old issue podkrepi-bg/infrastructure#17 in the infrastructure repo.
I don't know if it is still relevant though or if he is talking specifically about the infrastructure as code that is being deployed . In any case if it is needed in the infrastructure repo the solution should be similar

Notes:
There is a CREATE SCHEMA "public"; line in the init.sql file which I am unsure if is relevant in any way, but couldn't test it.

This is how the file structure in the pg-db container looks like:
image

Is that right considering we have 2 databases now @imilchev?

@github-actions
Copy link

github-actions bot commented Nov 10, 2022

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements 71.74% 1901/2650
🔴 Branches 46.44% 248/534
🔴 Functions 44.49% 222/499
🟡 Lines 69.84% 1686/2414

Test suite run success

180 tests passing in 64 suites.

Report generated by 🧪jest coverage report action from c717bc9

@dimitur2204 dimitur2204 changed the title 359 - Change keycloak configuration to now use the existing postgras … 359 - One postgres instance for both keycloak and general database Nov 10, 2022
@igoychev
Copy link
Contributor

nice and quick - thanks for helping Nizamov!
I was just thinking how we are going to do the migration now, because if we merge this it will redirect to the new empty database. So we need a migration script of some kind, don't we? - @imilchev what do you think?

docker-compose.yml Outdated Show resolved Hide resolved
Copy link
Member

@kachar kachar left a comment

Choose a reason for hiding this comment

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

Awesome upgrade @dimitur2204 🚀

@kachar
Copy link
Member

kachar commented Nov 16, 2022

I was just thinking how we are going to do the migration now, because if we merge this it will redirect to the new empty database. So we need a migration script of some kind, don't we? - @imilchev what do you think?

@igoychev I think this setup is only local via docker-compose, so the data exports will be for the testing data only, is that correct?

@dimitur2204
Copy link
Contributor Author

dimitur2204 commented Nov 16, 2022

I was just thinking how we are going to do the migration now, because if we merge this it will redirect to the new empty database. So we need a migration script of some kind, don't we? - @imilchev what do you think?

@igoychev I think this setup is only local via docker-compose, so the data exports will be for the testing data only, is that correct?

I am pretty sure this is the case for the code in this PR.

From what I understand from @imilchev his intention was to also do that on production though so that we only have 1 postgres instance to manage, however I think that will require help from him and everyone responsible for production!

@dimitur2204 dimitur2204 merged commit dc0dd8d into master Nov 17, 2022
@dimitur2204 dimitur2204 deleted the 359-improvement-using-external-database-keycloak branch November 17, 2022 12:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants