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

Production Release - Keycloak Update to 22.0 #770

Merged
merged 11 commits into from
Oct 19, 2023
Merged

Conversation

ericstumper
Copy link
Contributor

@ericstumper ericstumper commented Oct 19, 2023

No description provided.

@ericstumper ericstumper temporarily deployed to production October 19, 2023 10:34 — with GitHub Actions Inactive
@github-actions github-actions bot added the auto-updated Indicates that this pull request has been automatically updated with the latest commit messages. label Oct 19, 2023
@review-agent-prime
Copy link
Contributor

Overall, the pull request seems to be following standard conventions and practices, and it is well-structured. Here are my suggestions:

  1. Make sure to properly order the operations in migration files. Particularly, the public_User_alter_column_status/up.sql alters column before it's added in the 1697197135225_alter_table_public_User_add_column_status/up.sql. Please confirm the ordering is correct.

  2. Write down migrations for auto-generated migrations. It might be challenging to write down migration when ALTER TABLE commands are used, but it's important to include a rollback strategy.

For instance, the down migration for public_AppSettings_add_column_app/up.sql might be:

ALTER TABLE "public"."AppSettings" DROP COLUMN "app";

Remember, writing both up and down migrations enhances the version control system, allowing you to move forward and backward through different database states.

  1. In 1697662666960_modify_primarykey_public_AppSettings/up.sql and 1697662588510_modify_primarykey_public_AppSettings/up.sql sequence order of operations is crucial. It seems like these operations should be in reverse. The commit transaction is presented in the first file while you start the transaction in the second file. Running migrations one after another might lead to an error. Please review the sequence of these operations.

  2. In the 1697197640210_insert_into_public_UserStatus/up.sql migration file, there's a typo in the comment for the DELETED status: 'considering these user is deleted).' The right sentence would be 'All private information concerning this user is deleted.' Update the sentence for readability like so:

INSERT INTO "public"."UserStatus"("value", "comment") VALUES (E'DELETED', E'All private information concerning this user is deleted.');

Despite these minor issues, your code looks good and follows good coding practices. Keep it up!

@ericstumper ericstumper temporarily deployed to production October 19, 2023 10:40 — with GitHub Actions Inactive
@ericstumper ericstumper merged commit 7d0dd59 into production Oct 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-updated Indicates that this pull request has been automatically updated with the latest commit messages.
Projects
Development

Successfully merging this pull request may close these issues.

2 participants