-
Notifications
You must be signed in to change notification settings - Fork 16
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
fix(changelog): should exclude file contents, search indexes and mg_ fields to keep storage use down #4305
base: master
Are you sure you want to change the base?
Conversation
backend/molgenis-emx2-sql/src/main/java/org/molgenis/emx2/sql/ChangeLogUtils.java
Show resolved
Hide resolved
…hange_log_skip_file_contents
backend/molgenis-emx2-sql/src/main/java/org/molgenis/emx2/sql/ChangeLogUtils.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
public static String buildAuditTrigger(String schemaName, String tableName) { | ||
return """ | ||
CREATE TRIGGER %3$s_audit | ||
CREATE OR REPLACE TRIGGER %3$s_audit |
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.
these should not be a trigger when the trigger is add , should fail fast
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.
???
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.
the code assumes there is no trigger with this name , if there is a trigger with this name, something has gone (very) wrong and we should know , not suppress the error
@@ -220,7 +236,7 @@ static void executeMigrationFile(Database db, String sqlFile, String message) { | |||
jooq.settings().setQueryTimeout(THREE_MINUTES); | |||
String sql = new String(Migrations.class.getResourceAsStream(sqlFile).readAllBytes()); | |||
jooq.execute(sql); | |||
logger.debug(message + "(file = " + sqlFile); | |||
logger.info(message + "(file = " + sqlFile); |
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.
info ?
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.
currently we cannot see that migrations are executed. I think this is important enough to be 'info'
backend/molgenis-emx2-sql/src/main/java/org/molgenis/emx2/sql/SqlTableMetadata.java
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.
- should be marked as feature instead of fix as it changes behaviour ( and even the log )
- not sure if we need the feature anymore
I think we do. File contents are disturbing the changelog viewing and take much space. If we would want to log those it should be elsewhere. |
Quality Gate passedIssues Measures |
What are the main changes you did:
how to test:
notes: