-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fixed wrong column name #84
Conversation
WalkthroughThis pull request focuses on modifying SQL initialization scripts across multiple deployment configurations (Docker Compose, Docker, and Helm Chart). The primary change involves renaming the column Changes
Possibly related PRs
Suggested reviewers
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
The TipsCodeReview Commands (invoked as MR or PR comments)
CodeReview Discussion ChatThere are 2 ways to chat with Starship CodeReview:
Note: Be mindful of the bot's finite context window. CodeReview Documentation and Community
|
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.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-compose/init-scripts/initialize.sql
(1 hunks)docker/etc/server/initialize.sql
(1 hunks)helm-chart/charts/csghub/charts/server/initialize.sql
(1 hunks)
🔇 Additional comments (2)
docker/etc/server/initialize.sql (1)
105-105
: Same issue as in Helm Chart initialization script.
docker-compose/init-scripts/initialize.sql (1)
105-105
: Same issue as in Helm Chart initialization script.
@@ -102,7 +102,7 @@ BEGIN | |||
IF EXISTS ( | |||
SELECT FROM information_schema.columns | |||
WHERE table_name = 'runtime_frameworks' | |||
AND column_name = 'frame_npu_image_01' | |||
AND column_name = 'frame_npu_image' |
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.
🛠️ Refactor suggestion
Consider checking for both old and new column names during migration.
The EXISTS check only looks for the new column name frame_npu_image
. This could cause issues if the script runs on a database where the column hasn't been renamed yet.
Consider modifying the check to handle both column names:
- AND column_name = 'frame_npu_image'
+ AND column_name IN ('frame_npu_image_01', 'frame_npu_image')
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
AND column_name = 'frame_npu_image' | |
AND column_name IN ('frame_npu_image_01', 'frame_npu_image') |
The TipsCodeReview Commands (invoked as MR or PR comments)
CodeReview Discussion ChatThere are 2 ways to chat with Starship CodeReview:
Note: Be mindful of the bot's finite context window. CodeReview Documentation and Community
|
1 similar comment
The TipsCodeReview Commands (invoked as MR or PR comments)
CodeReview Discussion ChatThere are 2 ways to chat with Starship CodeReview:
Note: Be mindful of the bot's finite context window. CodeReview Documentation and Community
|
Refactor column name
frame_npu_image_01
toframe_npu_image_01
Summary by CodeRabbit
New Features
space_resources
,runtime_frameworks
, andruntime_architectures
tables.Bug Fixes
frame_npu_image_01
toframe_npu_image
in theruntime_frameworks
table, including adjustments to constraints.Chores