-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
feat: workspace:health nullable fix #3882
Conversation
@@ -188,16 +188,6 @@ export class RelationMetadataHealthService { | |||
}); | |||
} | |||
|
|||
if (relationColumn.isNullable !== relationFieldMetadata.isNullable) { |
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 needed as we're checking nullability on foreign keys in the field check
import { kebabCase } from 'src/utils/kebab-case'; | ||
|
||
@Injectable() | ||
export class CommandLogger { |
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.
Create some command logger logic here that can be shared between commands
this.logger.log('Running in dry run mode, rolling back transaction'); | ||
|
||
await queryRunner.rollbackTransaction(); | ||
|
||
await this.workspaceLogsService.saveLogs(storage, workspaceMigrations); |
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.
Move creation of logs file inside command, doesn't feel to be the correct place directly in the services
} | ||
|
||
try { | ||
await fs.writeFile( |
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 we use fileUploadService here to write to S3 on prod env?
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.
@Weiko Already discussed with @charlesBochet, when I've implemented it. As it's only here to write logs of command, we don't want to store this in s3, just locally :)
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.
LGTM!
This PR is implementing the
nullable
kind in the--fix
option ofworkspace:health
command.The core was created here #3863
Fix #3840