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

Backfill workspace version column #10637

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FelixMalfait
Copy link
Member

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

Copy link
Contributor

@greptile-apps greptile-apps bot left a 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';
Copy link
Contributor

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'

Suggested change
import { MigrationCommandModule } from 'src/database/commands/migration-command/miration-command.module';
import { MigrationCommandModule } from 'src/database/commands/migration-command/migration-command.module';

Comment on lines +41 to +43
this.logger.log(
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`),
);
Copy link
Contributor

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.

Suggested change
this.logger.log(
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`),
);
this.logger.log(
chalk.red(`Error in workspace ${workspaceId} - ${error.message}`),
);
throw error;

Comment on lines +7 to +9
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf8'));

return packageJson.version;
Copy link
Contributor

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

Suggested change
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;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant