RHOAIENG-25226: Standardize the image name with image convention #1070
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Change image name as per image name convention - Split params.env into latest and previous - Adjust runtime manifest in params format Signed-off-by: Harshad Reddy Nalla <[email protected]>
Signed-off-by: Harshad Reddy Nalla <[email protected]>
atheo89
reviewed
May 9, 2025
jiridanek
reviewed
May 9, 2025
atheo89
reviewed
May 9, 2025
atheo89
reviewed
May 9, 2025
Signed-off-by: Harshad Reddy Nalla <[email protected]>
3 tasks
harshad16
added
the
tide/merge-method-squash
Signed-off-by: Harshad Reddy Nalla <[email protected]>
Signed-off-by: Harshad Reddy Nalla <[email protected]>
jiridanek
reviewed
May 12, 2025
There was a problem hiding this comment.
Can you please add explanation into some readme or a comment on top of params.env about
- Change image name as per image name convention
- Split params.env into latest and previous
?
What are the image conventions, and why the env files have to be split?
# this file contains references to latest versions of workbench images that are updated by konflux nudges. see params.env for the references to older versions
something like that
and
Image names follow the established RHOAI format odh-<component>...-ubi9. Or whatever the standard image name actually is.
|
Is Konflux able to maintain the commit.envs, actually? I never saw it mentioned that nudges also update commit hashes! |
Signed-off-by: Harshad Reddy Nalla <[email protected]>
Signed-off-by: Harshad Reddy Nalla <[email protected]>
|
Updated all the required changes |
Co-authored-by: Jan Stourac <[email protected]>
Co-authored-by: Jan Stourac <[email protected]>
Signed-off-by: Harshad Reddy Nalla <[email protected]>
|
/lgtm |
|
Thanks @jiridanek @jstourac @atheo89 for the review 💯 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harshad16 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Merging to move forward |
atheo89
pushed a commit
to atheo89/notebooks
that referenced
this pull request
Jul 14, 2025
…flux/component-updates/component-update-odh-workbench-jupyter-trustyai-cpu-py311-ubi9-n-v2-23 Update odh-workbench-jupyter-trustyai-cpu-py311-ubi9-n-v2-23 to d901f7b
|
@coderabbitai ptal |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 3924 🏁 Script executed: Length of output: 8958 🏁 Script executed: Length of output: 2490 @jiridanek Thanks for asking me to review! This PR successfully implements a comprehensive standardization of image naming conventions across the notebooks repository. Here's my review: ✅ Strengths
📝 Observations
🎯 SummaryThis is a well-executed standardization effort that brings much-needed consistency to the image naming conventions. The implementation is thorough, properly documented, and maintains backward compatibility through the N-1 version strategy. The changes align perfectly with the RHOAIENG-25226 requirements and set a solid foundation for future workbench management. Great work by @harshad16 and the team! The systematic approach to this refactoring demonstrates excellent engineering practices. 🧠 Learnings used |
|
@coderabbitai are you sure this is not introducing any bugs you may've missed? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@jiridanek Let me perform a more thorough analysis to check for potential issues that might have been overlooked. @jiridanek After a thorough analysis, I found one potential bug that appears to still be present: 🐛 Duplicate Value Detection Bug in Validation ScriptThe validation script at This means duplicate image values could pass validation undetected, which could cause runtime issues if two different variables point to the same image. ✅ Other Aspects Look GoodThe rest of the implementation appears solid:
Would you like me to create a GitHub issue to track fixing the duplicate value detection bug? This is a pre-existing issue (not introduced by this PR) but it becomes more important with the doubled number of params/commit entries. 🏁 Script executed: Length of output: 2029 🏁 Script executed: Length of output: 4793 🏁 Script executed: Length of output: 10018 🏁 Script executed: Length of output: 1257 🏁 Script executed: Length of output: 51 🏁 Script executed: Length of output: 5938 🏁 Script executed: Length of output: 4095 @jiridanek After thorough analysis, I found two issues - one critical bug and one inconsistency: 🐛 Critical Bug: Duplicate Value DetectionLocation: # Current (WRONG - extracts variable names):
content=$(sed 's#\(.*\)=.*#\1#' "${env_file_path_1}"; sed 's#\(.*\)=.*#\1#' "${env_file_path_2}" | sort)
# Should be (extracts values):
content=$(sed 's#^[^=]*=\(.*\)#\1#' "${env_file_path_1}"; sed 's#^[^=]*=\(.*\)#\1#' "${env_file_path_2}" | sort)I confirmed this with a test:
This means if two different variables point to the same image SHA, it won't be detected as a duplicate, potentially causing runtime issues.
|
|
@coderabbitai filled we will fix it eventually |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2514 @jiridanek Thanks for confirming! I see issue #1786 has been created with comprehensive details about the duplicate value detection bug. Good to know it's tracked for eventual fixing. As you noted, this is a pre-existing issue (not introduced by PR #1070) but becomes more relevant with the doubled number of params/commit entries from the file splitting. The systematic tracking ensures it won't be forgotten. Let me know if you need any clarification on the issue details when you get to fixing it! 🧠 Learnings used |
|
@coderabbitai ofc. it is introduced in #1070, see change on line 66 in ci/check-params-env.sh |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1122 🏁 Script executed: Length of output: 547 🏁 Script executed: Length of output: 10373 @jiridanek You're absolutely right! I apologize for the error in my analysis. Looking at the diff, the bug was definitely introduced in PR #1070, not pre-existing: Before PR #1070 (line ~66): content=$(sed 's#.*=\(.*\)#\1#' "${env_file_path}" | sort)This correctly extracted values (everything after the After PR #1070 (line 72): content=$(sed 's#\(.*\)=.*#\1#' "${env_file_path_1}"; sed 's#\(.*\)=.*#\1#' "${env_file_path_2}" | sort)This incorrectly extracts variable names (everything before the The sed pattern was accidentally reversed when the function was modified to handle two files. The regex capture group Thank you for the correction - this is indeed a regression introduced by the standardization changes, making it more critical to fix since it's a new bug rather than a long-standing issue. Issue #1786 should ideally be updated to reflect that this was introduced in PR #1070. 🧠 Learnings used |
|
@coderabbitai add comment onto the issue |
4 tasks
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1258 @jiridanek Done! I've added a comment to issue #1786 clarifying that the bug was introduced in PR #1070, not pre-existing. The comment includes:
The comment is now visible at: #1786 (comment) This should help whoever picks up the fix understand exactly what went wrong and when. 🧠 Learnings used |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Standardize the image name with image convention
Description
Related-to: RHOAIENG-25226
How Has This Been Tested?
Merge criteria: