🌱 may want to use helm for installs#542
Merged
djzager merged 1 commit intokonveyor:mainfrom Mar 10, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds a USE_HELM flag and Helm-based installation path to the install-konveyor workflow, including new Helm-specific functions, conditional tool checks, adjusted debug output, and updates to the GitHub Action to accept a use_helm input. Changes
Sequence Diagram(s)sequenceDiagram
participant Script as Installation Script
participant Tool as Installer (Helm / OLM)
participant Kube as Kubernetes API
participant Op as Tackle Operator
alt USE_HELM = true
Script->>Tool: helm install tackle-operator
Tool->>Kube: create namespace & deploy operator resources
Script->>Kube: poll operator deployment readiness
Kube-->>Script: deployment available
Script->>Kube: create Tackle CR
Kube->>Op: operator reconciles CR -> deploys Tackle resources
Script->>Kube: validate deployments & services
Kube-->>Script: stack ready
else USE_HELM = false
Script->>Tool: install OLM + operator via operator-sdk/bundle
Tool->>Kube: install OLM components & operator
Script->>Kube: poll operator readiness
Kube-->>Script: operator available
Script->>Kube: create Tackle CR
Kube->>Op: operator reconciles CR -> deploys Tackle resources
Script->>Kube: validate deployments & services
Kube-->>Script: stack ready
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/install-konveyor.sh (1)
364-364: Static analysis: Declare and assign separately to avoid masking return values.ShellCheck flags this line because combining
localwith command substitution can mask the command's exit status. However, this pattern is used consistently throughout the script (lines 181, 221, 271), andget_remaining_timeis a simple arithmetic function unlikely to fail.Given the project's preference for keeping hack scripts simple, this is acceptable as-is, but if you want to address it:
♻️ Optional fix
- local remaining=$(get_remaining_time) + local remaining + remaining=$(get_remaining_time)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/install-konveyor.sh` at line 364, The assignment "local remaining=$(get_remaining_time)" can mask the exit status of get_remaining_time; change it to declare then assign separately by first doing "local remaining" and then "remaining=$(get_remaining_time)" so the command substitution's exit code isn't masked; update every similar occurrence (e.g., uses of remaining and calls to get_remaining_time) to follow this pattern to satisfy ShellCheck and preserve correct error propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/install-konveyor.sh`:
- Line 364: The assignment "local remaining=$(get_remaining_time)" can mask the
exit status of get_remaining_time; change it to declare then assign separately
by first doing "local remaining" and then "remaining=$(get_remaining_time)" so
the command substitution's exit code isn't masked; update every similar
occurrence (e.g., uses of remaining and calls to get_remaining_time) to follow
this pattern to satisfy ShellCheck and preserve correct error propagation.
Signed-off-by: David Zager <david.j.zager@gmail.com>
djzager
force-pushed
the
install-konveyor-helm
branch
from
7c97bf0 to
e5053a6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
hack/install-konveyor.sh (1)
363-368: Consider separating variable declaration and assignment.Static analysis flags SC2155 on line 364. While
get_remaining_timeis unlikely to fail, separating declaration and assignment is a shell best practice to avoid masking potential return values.♻️ Suggested fix
while true; do - local remaining=$(get_remaining_time) + local remaining + remaining=$(get_remaining_time) if [ $remaining -le 0 ]; thenNote: This same pattern exists in other functions (lines 181, 221, 271). Consider applying consistently if you choose to address this.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/install-konveyor.sh` around lines 363 - 368, The shell uses a combined declaration+assignment for "local remaining=$(get_remaining_time)" which triggers SC2155; change it to separate the declaration and assignment in the loop: first "local remaining" (or reuse an existing local outside the loop), then "remaining=$(get_remaining_time)" and check the command exit status (e.g., if ! remaining=$(get_remaining_time); then handle error) before using "$remaining"; apply the same change to the other occurrences that call get_remaining_time in this script to satisfy SC2155.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/install-konveyor.sh`:
- Around line 363-368: The shell uses a combined declaration+assignment for
"local remaining=$(get_remaining_time)" which triggers SC2155; change it to
separate the declaration and assignment in the loop: first "local remaining" (or
reuse an existing local outside the loop), then
"remaining=$(get_remaining_time)" and check the command exit status (e.g., if !
remaining=$(get_remaining_time); then handle error) before using "$remaining";
apply the same change to the other occurrences that call get_remaining_time in
this script to satisfy SC2155.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Fixes #513
Summary by CodeRabbit
New Features
Improvements