Conversation
…with-existing-cluster' Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
…e tests Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
WalkthroughThis update introduces new automation scripts and configuration for managing container image builds, Git operations, and manifest updates for KServe development workflows. It also modifies workflow triggers for KIND cluster lifecycle tests, removes a logging line from an E2E test runner, and comments out the ROSA install step in a playbook. Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant Script as create-devflag/main.sh
participant Git as GitHub Repo
participant Registry as Container Registry
Dev->>Script: Run automation with env variables
Script->>Script: Validate environment (validation.sh)
Script->>Git: Clone/setup repo & checkout branch (build.sh)
alt Custom image
Script->>Registry: Use provided image
else Build image
Script->>Registry: Build & push image (build.sh)
end
Script->>Git: Update manifests & push branch (build.sh)
Script->>Script: Run verifications (verify.sh)
Script->>Dev: Output manifest tarball URL
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (10)
unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml (1)
70-72: Avoid commented-out YAML; use a conditional flag to disable the roleCommenting out list items works but gradually clutters the playbook and risks merge conflicts. Prefer keeping the step active and gating execution with a
when: false(or a dedicated variable) so the structure remains intact and re-enabling is one-line.- # - role: - # name: openshift-rosa-install - # description: Install ROSA Openshift Cluster + - role: + name: openshift-rosa-install + description: Install ROSA Openshift Cluster + when: false # temporarily disabledThis keeps YAML valid, self-documenting, and easier to toggle.
src/roles/kserve/tests/create-devflag/config.yaml (2)
1-3: Directory name violates repository naming conventionThe coding guidelines mandate lowercase with underscores. The directory
create-devflag/contains a hyphen.
Consider renaming it tocreate_devflag/for consistency across the repo.
18-23: Remove trailing whitespace to satisfy YAML-lintLines 18, 20 and 23 contain superfluous spaces flagged by YAML-lint.
Whitespace at EOL adds churn in diffs and may break strict CI linters.... ...src/roles/kserve/tests/create-devflag/main.sh (2)
33-34:role_nameis never usedShellcheck SC2034 is right; remove the dead variable to keep the script tidy.
75-80:exitrenders following code unreachable
result=1after anexitwill never execute. Delete or move the assignment beforeexit.src/roles/kserve/tests/create-devflag/validation.sh (1)
7-8: Utility script sourced on every call – move outside function
utils.shis sourced each timevalidate_env_varsruns, which is unnecessary overhead and can lead to double definition of helpers. Source it once at file scope instead.src/roles/kserve/tests/create-devflag/verify.sh (1)
47-49:repo_nameparameter is unusedThe third argument is accepted but never referenced; drop it or implement its intended use to avoid future confusion.
src/roles/kserve/tests/create-devflag/build.sh (3)
10-16: Misleading comment: looking for “.git” but messaging refers to “.github”
The loop correctly searches for a.gitdirectory, yet the surrounding comments and the error message mention “.github”. This is confusing for future maintainers – please align the wording with the actual behaviour (or vice-versa).
88-92: Unquoted operands in string comparison – may expand globs (SC2053)
Guard against pathname expansion by quoting both sides:-if [[ $(basename "$ORIGINAL_DIR") != ${COMPONENT_NAME} ]]; then +if [[ "$(basename "$ORIGINAL_DIR")" != "${COMPONENT_NAME}" ]]; then
61-64: Hard-coded local image reference may break multi-arch or non-Docker workflows
The tag step assumesdocker.io/library/${component}-controller:latestexists locally. In environments using Podman, a customTAG, or a multi-arch build, this may be incorrect. Consider deriving the local image reference from themake docker-buildtarget (e.g. via an output variable) or tag directly from the build output to avoid coupling to the Docker Hub namespace.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/e2e-cluster-lifecycle.yml(1 hunks)src/roles/kserve/tests/create-devflag/build.sh(1 hunks)src/roles/kserve/tests/create-devflag/config.yaml(1 hunks)src/roles/kserve/tests/create-devflag/main.sh(1 hunks)src/roles/kserve/tests/create-devflag/validation.sh(1 hunks)src/roles/kserve/tests/create-devflag/verify.sh(1 hunks)src/roles/kserve/tests/e2e-test/main.sh(0 hunks)unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yaml(1 hunks)
💤 Files with no reviewable changes (1)
- src/roles/kserve/tests/e2e-test/main.sh
🧰 Additional context used
📓 Path-based instructions (1)
`**/*`: Use lowercase with underscores for directories and files (e.g., routers/user_routes.py).
**/*: Use lowercase with underscores for directories and files (e.g., routers/user_routes.py).
unofficial_components/playbooks/odh-test/kserve-e2e-test-with-existing-cluster/config.yamlsrc/roles/kserve/tests/create-devflag/main.shsrc/roles/kserve/tests/create-devflag/verify.shsrc/roles/kserve/tests/create-devflag/build.shsrc/roles/kserve/tests/create-devflag/config.yamlsrc/roles/kserve/tests/create-devflag/validation.sh
🪛 Shellcheck (0.10.0)
src/roles/kserve/tests/create-devflag/main.sh
[warning] 33-33: role_name appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 98-98: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
src/roles/kserve/tests/create-devflag/verify.sh
[warning] 48-48: repo_name appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 50-50: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
src/roles/kserve/tests/create-devflag/build.sh
[warning] 73-73: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 86-86: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
[warning] 88-88: Quote the right-hand side of != in [[ ]] to prevent glob matching.
(SC2053)
[warning] 183-183: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🪛 YAMLlint (1.37.1)
src/roles/kserve/tests/create-devflag/config.yaml
[error] 18-18: trailing spaces
(trailing-spaces)
[error] 20-20: trailing spaces
(trailing-spaces)
[error] 23-23: trailing spaces
(trailing-spaces)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: kind-cluster-lifecycle-test (3.13)
- GitHub Check: cluster-test (3.12)
- GitHub Check: non-cluster-test (3.12)
🔇 Additional comments (1)
.github/workflows/e2e-cluster-lifecycle.yml (1)
24-27: Confirm the exclude-list still matches desired paths
default_provided_services/was removed from the regexp.
If any cluster-lifecycle relevant code now lives there, the tests will silently skip.Request verification that this omission is intentional and documented.
| cd $GIT_PARENT_REPO_DIR/${COMPONENT_NAME} | ||
| # Check and update controller image in params.env |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to cd
A failing cd will silently continue in the wrong directory.
-cd $GIT_PARENT_REPO_DIR/${COMPONENT_NAME}
+cd "$GIT_PARENT_REPO_DIR/${COMPONENT_NAME}" || { error "Unable to cd into repo dir"; exit 1; }📝 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.
| cd $GIT_PARENT_REPO_DIR/${COMPONENT_NAME} | |
| # Check and update controller image in params.env | |
| cd "$GIT_PARENT_REPO_DIR/${COMPONENT_NAME}" || { error "Unable to cd into repo dir"; exit 1; } | |
| # Check and update controller image in params.env |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 98-98: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/main.sh at lines 98-99, the script
changes directory using `cd` without checking if it succeeded. Add error
handling by testing the exit status of the `cd` command immediately after it
runs, and if it fails, print an error message and exit the script to prevent
continuing in the wrong directory.
| source $root_directory/src/commons/scripts/utils.sh | ||
| ## INIT END ## |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Quote sourced path & check existence
Unquoted $root_directory can break when the path contains spaces; sourcing a non-existent file will abort.
-source $root_directory/src/commons/scripts/utils.sh
+UTILS_FILE="${root_directory}/src/commons/scripts/utils.sh"
+[[ -f "$UTILS_FILE" ]] || { echo "utils.sh not found at $UTILS_FILE" >&2; exit 1; }
+source "$UTILS_FILE"📝 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.
| source $root_directory/src/commons/scripts/utils.sh | |
| ## INIT END ## | |
| # before: | |
| # source $root_directory/src/commons/scripts/utils.sh | |
| # ## INIT END ## | |
| # after: | |
| UTILS_FILE="${root_directory}/src/commons/scripts/utils.sh" | |
| [[ -f "$UTILS_FILE" ]] || { | |
| echo "utils.sh not found at $UTILS_FILE" >&2 | |
| exit 1 | |
| } | |
| source "$UTILS_FILE" | |
| ## INIT END ## |
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/main.sh around lines 23 to 24, the
sourced path $root_directory/src/commons/scripts/utils.sh is unquoted, which can
cause issues if $root_directory contains spaces, and the script does not check
if the file exists before sourcing. Fix this by quoting the entire path to
handle spaces properly and add a check to verify the file exists before sourcing
it to prevent the script from aborting unexpectedly.
| if [[ $DEBUG == "0" ]]; then | ||
| set -x | ||
| fi |
There was a problem hiding this comment.
DEBUG guard inverted – set -x enabled when DEBUG==0
Usually tracing is enabled for non-zero / truthy DEBUG. Current logic does the opposite.
-if [[ $DEBUG == "0" ]]; then
- set -x
-fi
+if [[ "${DEBUG:-0}" != "0" ]]; then
+ set -x
+fi📝 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.
| if [[ $DEBUG == "0" ]]; then | |
| set -x | |
| fi | |
| if [[ "${DEBUG:-0}" != "0" ]]; then | |
| set -x | |
| fi |
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/main.sh at lines 3 to 5, the condition
enabling set -x is inverted; it currently enables tracing when DEBUG equals "0".
Change the condition to enable set -x only when DEBUG is non-zero or truthy, for
example by checking if DEBUG is not equal to "0" or if DEBUG is set and
non-empty.
| # Get local image SHA | ||
| local_sha=$(${CONTAINER_CLI} inspect docker.io/library/${COMPONENT_NAME}-controller:latest | jq -r '.[0].Id' 2>/dev/null) | ||
| if [[ -z "$local_sha" || "$local_sha" == "null" ]]; then | ||
| error "Failed to get local image SHA" | ||
| return 1 | ||
| fi |
There was a problem hiding this comment.
Local SHA comparison uses hard-coded docker.io/library image
The built image is pushed to ${REGISTRY_URL}/${COMPONENT_NAME}-controller:${CTRL_IMG_TAG}; inspecting docker.io/library/...:latest will almost always mismatch.
- local_sha=$(${CONTAINER_CLI} inspect docker.io/library/${COMPONENT_NAME}-controller:latest | jq -r '.[0].Id' 2>/dev/null)
+ local_sha=$(${CONTAINER_CLI} inspect "$image_url" | jq -r '.[0].Id' 2>/dev/null)This aligns SHA comparison with the actual image pushed.
📝 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.
| # Get local image SHA | |
| local_sha=$(${CONTAINER_CLI} inspect docker.io/library/${COMPONENT_NAME}-controller:latest | jq -r '.[0].Id' 2>/dev/null) | |
| if [[ -z "$local_sha" || "$local_sha" == "null" ]]; then | |
| error "Failed to get local image SHA" | |
| return 1 | |
| fi | |
| # Get local image SHA | |
| local_sha=$(${CONTAINER_CLI} inspect "$image_url" | jq -r '.[0].Id' 2>/dev/null) | |
| if [[ -z "$local_sha" || "$local_sha" == "null" ]]; then | |
| error "Failed to get local image SHA" | |
| return 1 | |
| fi |
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/verify.sh around lines 28 to 33, the
local image SHA is being retrieved by inspecting a hard-coded image reference
using docker.io/library, which does not match the actual image pushed. Update
the image reference in the inspect command to use the variable
${REGISTRY_URL}/${COMPONENT_NAME}-controller:${CTRL_IMG_TAG} instead of
docker.io/library/${COMPONENT_NAME}-controller:latest to ensure the SHA
comparison aligns with the correct pushed image.
| cd $repo_dir | ||
|
|
||
| if git remote show ${user_name} >/dev/null 2>&1; then | ||
| info "Username remote found: ${user_name}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard cd with failure check
cd $repo_dir will exit the subshell on error but continue verification producing misleading results.
-cd $repo_dir
+cd "$repo_dir" || { error "Cannot cd into $repo_dir"; return 1; }📝 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.
| cd $repo_dir | |
| if git remote show ${user_name} >/dev/null 2>&1; then | |
| info "Username remote found: ${user_name}" | |
| cd "$repo_dir" || { error "Cannot cd into $repo_dir"; return 1; } | |
| if git remote show ${user_name} >/dev/null 2>&1; then | |
| info "Username remote found: ${user_name}" |
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 50-50: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/verify.sh around lines 50 to 53, the
command `cd $repo_dir` is not checked for failure, which can cause the script to
continue running with misleading results if the directory change fails. Add a
check immediately after `cd $repo_dir` to verify it succeeded, and if not, exit
the script or handle the error appropriately to prevent further execution.
| if [[ "${use_case}" != "LOCAL_SOURCE" ]]; then | ||
| cd $target_parent_directory | ||
|
|
||
| # Clone repository | ||
| if [[ -d ${COMPONENT_NAME} ]]; then |
There was a problem hiding this comment.
🛠️ Refactor suggestion
cd commands are unchecked – script may operate in an unexpected directory
ShellCheck SC2164 flags these instances. Add || exit 1 (or || return 1 inside functions) to fail fast when the target directory is missing:
-cd "$target_parent_directory"
+cd "$target_parent_directory" || return 1
...
-cd ${COMPONENT_NAME}
+cd "${COMPONENT_NAME}" || return 1
...
-cd "$target_parent_directory/${COMPONENT_NAME}"
+cd "$target_parent_directory/${COMPONENT_NAME}" || return 1Also applies to: 86-87, 183-184
🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 73-73: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/build.sh around lines 72 to 76, the cd
commands lack error checking, which can cause the script to continue running in
the wrong directory if the cd fails. To fix this, append || exit 1 to each cd
command outside functions to ensure the script exits immediately if changing
directories fails. Apply the same fix to the cd commands at lines 86-87 and
183-184.
| if [[ $DEBUG == "0" ]]; then | ||
| set -x | ||
| fi |
There was a problem hiding this comment.
DEBUG flag logic looks inverted and unquoted – trace will be enabled when DEBUG is “0”
set -x is usually activated when a truthy / non-zero DEBUG is supplied. As written, the script goes verbose for the “off” value and is silent otherwise. Additionally the variable is unquoted.
-if [[ $DEBUG == "0" ]]; then
- set -x
-fi
+if [[ "${DEBUG}" == "1" ]]; then # enable tracing only when explicitly requested
+ set -x
+fi📝 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.
| if [[ $DEBUG == "0" ]]; then | |
| set -x | |
| fi | |
| if [[ "${DEBUG}" == "1" ]]; then # enable tracing only when explicitly requested | |
| set -x | |
| fi |
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/build.sh around lines 3 to 5, the DEBUG
flag logic is inverted and the variable is unquoted. To fix this, change the
condition to enable set -x when DEBUG is not "0" (i.e., when DEBUG is truthy),
and quote the variable in the test to prevent word splitting or errors. This
means using a condition like if [[ "$DEBUG" != "0" ]]; then set -x; fi.
| if [ -d "$github_dir/.git" ]; then | ||
| root_directory="$github_dir" | ||
| else | ||
| echo "Error: Unable to find .github folder." | ||
| fi | ||
| source $root_directory/src/commons/scripts/utils.sh | ||
| ## INIT END ## |
There was a problem hiding this comment.
Script continues after root-repo discovery failure – will dereference an uninitialised variable
If the .git folder isn’t found, root_directory stays unset, yet the script still executes source $root_directory/..., leading to “No such file or directory”. Bail out early and quote the variable when sourcing:
-else
- echo "Error: Unable to find .github folder."
-fi
-source $root_directory/src/commons/scripts/utils.sh
+else
+ echo "Error: Unable to find .git folder. Aborting."
+ exit 1
+fi
+source "${root_directory}/src/commons/scripts/utils.sh"📝 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.
| if [ -d "$github_dir/.git" ]; then | |
| root_directory="$github_dir" | |
| else | |
| echo "Error: Unable to find .github folder." | |
| fi | |
| source $root_directory/src/commons/scripts/utils.sh | |
| ## INIT END ## | |
| if [ -d "$github_dir/.git" ]; then | |
| root_directory="$github_dir" | |
| else | |
| echo "Error: Unable to find .git folder. Aborting." | |
| exit 1 | |
| fi | |
| source "${root_directory}/src/commons/scripts/utils.sh" | |
| ## INIT END ## |
🤖 Prompt for AI Agents
In src/roles/kserve/tests/create-devflag/build.sh around lines 17 to 23, the
script continues after failing to find the .git folder, causing root_directory
to be unset and leading to an error when sourcing utils.sh. Fix this by adding
an exit command immediately after the error message to stop the script, and
quote the root_directory variable in the source command to handle any spaces
safely.
|
/lgtm |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Chores
Style