-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Backfill workspace version column #10637
base: main
Are you sure you want to change the base?
Conversation
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.
PR Summary
This PR adds workspace versioning functionality by introducing a version field for workspaces and a migration command to backfill existing workspaces with version data.
- Critical: Version field missing from insert columns in
workspaces.ts
seed data, causing version data to be lost during insertion - Brittle path resolution in
version.util.ts
using relative path../../../../package.json
could break if file structure changes getAppVersion()
has performance concerns with synchronous file reads and lacks caching- Migration command may need to be updated from 0.43 to 0.44 based on timing
- Consider adding version string validation in
version.util.ts
to ensure proper format
5 file(s) reviewed, 3 comment(s)
Edit PR Review Bot Settings | Greptile
@@ -4,6 +4,7 @@ import { TypeOrmModule } from '@nestjs/typeorm'; | |||
import { MigrationCommandModule } from 'src/database/commands/migration-command/miration-command.module'; |
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.
syntax: Typo in import path: 'miration-command.module' should be 'migration-command.module'
import { MigrationCommandModule } from 'src/database/commands/migration-command/miration-command.module'; | |
import { MigrationCommandModule } from 'src/database/commands/migration-command/migration-command.module'; |
this.logger.log( | ||
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`), | ||
); |
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.
logic: Error is only logged but not propagated. Could lead to silent failures in the migration process.
this.logger.log( | |
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`), | |
); | |
this.logger.log( | |
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`), | |
); | |
throw error; |
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | ||
|
||
return packageJson.version; |
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.
logic: No validation that packageJson.version exists or is in valid semver format
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | |
return packageJson.version; | |
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8')); | |
return typeof packageJson.version === 'string' ? packageJson.version : null; |
This probably comes too late for 0.43? I had this in my stash and forgot to raise a PR before leaving last week.
I can update it to 0.44 if it's too late