Tackle script#202
Conversation
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new GitHub Actions workflow ( Changes
Sequence DiagramsequenceDiagram
participant GHA as GitHub Actions
participant Script as tackle.sh
participant Kind as Kind Cluster
participant K8s as Kubernetes
participant Tackle as Tackle Services
participant KC as Keycloak
GHA->>Script: install-deps
Script->>Script: Check/Install kind, kubectl
GHA->>Script: install --auth/--no-auth
Script->>Kind: Create Kind cluster
Script->>K8s: Configure local-path-storage
Script->>K8s: Install ingress-nginx
Script->>K8s: Install OLM
Script->>K8s: Deploy Tackle operator
Script->>Tackle: Apply Tackle CR (with images, cache)
alt Authentication enabled
Script->>KC: Wait for Keycloak
Script->>K8s: Apply NetworkPolicy
Script->>KC: Configure hostname/env
Script->>KC: Clear required actions
end
Script->>K8s: Port-forward Hub service
GHA->>Script: status
Script->>K8s: Query cluster state
Script->>Tackle: Get logs/pods/services
GHA->>Script: uninstall
Script->>K8s: Delete Tackle CR
Script->>Kind: Delete cluster
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
scripts/tackle.sh (1)
579-617: Remove the duplicatecmdStatusdefinition.This second copy silently overrides the implementation already declared at Line 133. Keeping both makes future fixes easy to land in the dead copy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/tackle.sh` around lines 579 - 617, The file contains a duplicate function definition for cmdStatus that overrides the original; remove this second copy (the duplicate cmdStatus block) so only the initial implementation remains, ensuring any required statements used elsewhere (e.g., the final success "Tackle status check complete" and calls to runKubectl) are preserved from the original implementation and not lost during removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test-tackle.yaml:
- Around line 8-10: The reusable workflow call under the build-images job uses
"uses: jortel/tackle2-hub/..." with "secrets: inherit", which won't forward
secrets across repository owners; replace the inherit usage by listing the
required secrets explicitly in a secrets mapping (e.g., provide DOCKER_USERNAME,
DOCKER_PASSWORD, REGISTRY_TOKEN, or whichever registry/GHA secrets the called
workflow expects) so the called workflow receives credentials; update the
build-images job's secrets stanza to enumerate those secret names instead of
using inherit.
- Around line 33-37: The reusable workflow chain is not exporting the built
image tags, so add outputs called hub-image and analyzer-image at each level:
modify build-nightly-images.yaml to set outputs with the actual image names/tags
produced by its build steps (e.g., capture the step output or job output into
outputs.hub-image and outputs.analyzer-image), update e2e-image-build.yaml (the
workflow invoked by build-images) to re-export those outputs from
build-nightly-images as its own outputs, and finally update the top-level
workflow (tackle2-hub/main.yml) to expose hub-image and analyzer-image so the
test-tackle.yaml job (which references needs.build-images.outputs.hub-image and
analyzer-image) receives the real built tags instead of falling back to the
format(...) patterns; ensure the outputs names match exactly (hub-image,
analyzer-image) across all workflows and wire them to the correct step/job
outputs.
In `@scripts/tackle.sh`:
- Around line 679-681: The case handling discards/doesn't forward CLI args so
only cmdInstall sees --cluster; update the case branches so cmdStatus and
cmdUninstall receive the original positional/flag args (and cmdInstall still
gets them) by forwarding "$@" to them (and removing any unnecessary shift that
drops flags) so that cmdStatus and cmdUninstall can parse --cluster like
cmdInstall; refer to the case labels and functions cmdInstall, cmdStatus, and
cmdUninstall when making the change.
- Around line 574-576: The script currently calls startPortForward (which runs
foreground and blocks) unconditionally after install, preventing later steps
like status or uninstall from running; change this so startPortForward is either
opt-in or skipped in non-interactive sessions by guarding the call with an
interactive check or feature flag (e.g., only call startPortForward when a
PORT_FORWARD=true flag/option is set or when stdout is a TTY via an interactive
test like [ -t 1 ]), and ensure any CLI/install path or CI environment skips the
foreground port-forward so the workflow can continue to status/uninstall.
- Around line 67-84: The waitFor helper (function waitFor) currently increments
i by fixed 5s and doesn't enforce a real wall-clock budget because callers
(e.g., kubectl wait --timeout=...) can block per-attempt; change waitFor to
track a start time and compute remaining_seconds each loop iteration, then run
the probe command with that per-attempt timeout (e.g., wrap the probe in timeout
or adjust kubectl --timeout) so no single probe can exceed the remaining budget,
break when remaining_seconds <= 0 and die with timeout; ensure sleep uses min(5,
remaining_seconds) and keep the success/failure logic in waitFor unchanged.
---
Nitpick comments:
In `@scripts/tackle.sh`:
- Around line 579-617: The file contains a duplicate function definition for
cmdStatus that overrides the original; remove this second copy (the duplicate
cmdStatus block) so only the initial implementation remains, ensuring any
required statements used elsewhere (e.g., the final success "Tackle status check
complete" and calls to runKubectl) are preserved from the original
implementation and not lost during removal.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 18defe33-35c7-4132-803b-120deb5f2de7
📒 Files selected for processing (16)
.github/saved-workflows/build-nightly-images.yaml.github/saved-workflows/ci-repo.yaml.github/saved-workflows/e2e-hub-koncur.yaml.github/saved-workflows/e2e-image-build.yaml.github/saved-workflows/e2e-kantra-koncur.yaml.github/saved-workflows/global-ci-bundle.yml.github/saved-workflows/global-ci.yml.github/saved-workflows/nightly-koncur-0.9.yaml.github/saved-workflows/nightly-koncur.yaml.github/saved-workflows/nightly-main.yaml.github/saved-workflows/nightly-matrix-config.yaml.github/saved-workflows/nightly-release-0.7.yaml.github/saved-workflows/nightly-release-0.8.yaml.github/saved-workflows/validate-shared-tests.yml.github/workflows/test-tackle.yamlscripts/tackle.sh
| build-images: | ||
| uses: jortel/tackle2-hub/.github/workflows/main.yml@056702a8b3ab4b4d3f9bf0067920c0febb5520e6 | ||
| secrets: inherit |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does GitHub Actions support secrets: inherit when a workflow in one repository owner or organization calls a reusable workflow from a different owner or organization?
💡 Result:
No, GitHub Actions does not support secrets: inherit when a workflow in one repository owner or organization calls a reusable workflow from a different owner or organization. The secrets: inherit keyword is explicitly limited to calls within the same organization or enterprise. Official GitHub documentation states: "Workflows that call reusable workflows in the same organization or enterprise can use the inherit keyword to implicitly pass the secrets." Examples use the same organization prefix (e.g., octo-org/example-repo). For cross-organization calls, secrets must be passed explicitly by name in the secrets object, and the reusable workflow must declare them in on.workflow_call.secrets. Organization-level secrets from the reusable workflow's repository are not accessible; only the caller's secrets can be passed. As of 2026-03-20, no updates enable cross-org inheritance, per docs and discussions.
Citations:
- 1: https://docs.github.com/en/actions/how-tos/reuse-automations/reuse-workflows
- 2: https://docs.github.com/en/actions/how-tos/sharing-automations/reusing-workflows#:~:text=Passing inputs and secrets to,keyword to pass named secrets.
- 3: https://docs.github.com/enterprise-cloud@latest/actions/using-workflows/reusing-workflows
- 4: https://docs.github.com/en/enterprise-cloud@latest/actions/using-workflows/sharing-workflows-secrets-and-runners-with-your-organization
secrets: inherit does not work across different repository owners—registry credentials will not be passed to the called workflow.
GitHub Actions only supports secrets: inherit for reusable workflows called within the same organization or enterprise. This call crosses owners (konveyor/ci → jortel/tackle2-hub), so inherited secrets are not accessible to the called workflow. Pass required secrets explicitly by name in the secrets object.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-tackle.yaml around lines 8 - 10, The reusable
workflow call under the build-images job uses "uses: jortel/tackle2-hub/..."
with "secrets: inherit", which won't forward secrets across repository owners;
replace the inherit usage by listing the required secrets explicitly in a
secrets mapping (e.g., provide DOCKER_USERNAME, DOCKER_PASSWORD, REGISTRY_TOKEN,
or whichever registry/GHA secrets the called workflow expects) so the called
workflow receives credentials; update the build-images job's secrets stanza to
enumerate those secret names instead of using inherit.
| env: | ||
| # Adjust based on actual outputs / tag pattern from build-images job | ||
| HUB: ${{ needs.build-images.outputs.hub-image || format('quay.io/konveyor/tackle2-hub:pr-{0}-{1}', github.event.number || github.sha, github.sha) }} | ||
| ANALYZER_ADDON: ${{ needs.build-images.outputs.analyzer-image || format('quay.io/konveyor/tackle2-addon-analyzer:pr-{0}-{1}', github.event.number || github.sha, github.sha) }} | ||
| # add more if needed |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ref=056702a8b3ab4b4d3f9bf0067920c0febb5520e6
url="https://raw.githubusercontent.com/jortel/tackle2-hub/${ref}/.github/workflows/main.yml"
tmp="$(mktemp)"
curl -fsSL "$url" -o "$tmp"
echo "=== workflow_call / outputs ==="
sed -n '/workflow_call:/,/^[^[:space:]]/p' "$tmp"
echo
echo "=== image output references ==="
rg -n 'hub-image|analyzer-image|outputs:|pr-' "$tmp" || trueRepository: konveyor/ci
Length of output: 119
🏁 Script executed:
# Try to access the tackle2-hub repository and check main.yml structure
curl -fsSL "https://api.github.com/repos/jortel/tackle2-hub/contents/.github/workflows/main.yml" | jq -r '.download_url' | head -1Repository: konveyor/ci
Length of output: 103
🏁 Script executed:
# Check if we can find the default branch's main.yml
curl -fsSL "https://raw.githubusercontent.com/jortel/tackle2-hub/main/.github/workflows/main.yml" -o /tmp/main_default.yml 2>&1 && echo "=== SUCCESS ===" && head -100 /tmp/main_default.yml || echo "=== FAILED with main branch ==="Repository: konveyor/ci
Length of output: 135
🏁 Script executed:
# Verify the commit exists and try to fetch from it
curl -fsSL "https://raw.githubusercontent.com/jortel/tackle2-hub/056702a8b3ab4b4d3f9bf0067920c0febb5520e6/.github/workflows/main.yml" -w "\nHTTP Status: %{http_code}\n" -o /tmp/main_commit.yml && cat /tmp/main_commit.yml | head -150Repository: konveyor/ci
Length of output: 1833
🏁 Script executed:
curl -fsSL "https://raw.githubusercontent.com/konveyor/ci/main/.github/workflows/e2e-image-build.yaml" -o /tmp/e2e_build.yml 2>&1 && echo "=== e2e-image-build.yaml ===" && cat /tmp/e2e_build.ymlRepository: konveyor/ci
Length of output: 6563
🏁 Script executed:
cd /tmp && git clone --depth 1 https://github.com/konveyor/ci.git 2>&1 | head -20 && cat /tmp/ci/.github/workflows/test-tackle.yaml | head -50Repository: konveyor/ci
Length of output: 229
🏁 Script executed:
find /tmp/ci -name "test-tackle.yaml" -o -name "*tackle*" | head -20Repository: konveyor/ci
Length of output: 80
🏁 Script executed:
cd /tmp && rm -rf ci && git clone --depth 1 https://github.com/konveyor/ci.git 2>&1 && ls -la ci/.github/workflows/ | grep -i tackleRepository: konveyor/ci
Length of output: 137
🏁 Script executed:
# Search for the file more broadly
git -C /tmp/ci ls-files | grep -i tackleRepository: konveyor/ci
Length of output: 37
🏁 Script executed:
curl -fsSL "https://raw.githubusercontent.com/konveyor/ci/main/.github/workflows/build-nightly-images.yaml" 2>&1 | head -150Repository: konveyor/ci
Length of output: 5226
Add outputs to the reusable workflow chain to propagate built image tags.
The build-images job uses konveyor/ci/.github/workflows/e2e-image-build.yaml which calls build-nightly-images.yaml, but neither workflow exports the actual image names/tags that were built. Without hub-image and analyzer-image outputs, the fallback format() expressions are always used, meaning the workflow deploys whatever tags were specified in the fallback pattern rather than the images that were actually built. Add outputs at each level: build-nightly-images.yaml should export the image names/tags, e2e-image-build.yaml should re-export them, and tackle2-hub/main.yml should expose them for test-tackle.yaml to consume.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test-tackle.yaml around lines 33 - 37, The reusable
workflow chain is not exporting the built image tags, so add outputs called
hub-image and analyzer-image at each level: modify build-nightly-images.yaml to
set outputs with the actual image names/tags produced by its build steps (e.g.,
capture the step output or job output into outputs.hub-image and
outputs.analyzer-image), update e2e-image-build.yaml (the workflow invoked by
build-images) to re-export those outputs from build-nightly-images as its own
outputs, and finally update the top-level workflow (tackle2-hub/main.yml) to
expose hub-image and analyzer-image so the test-tackle.yaml job (which
references needs.build-images.outputs.hub-image and analyzer-image) receives the
real built tags instead of falling back to the format(...) patterns; ensure the
outputs names match exactly (hub-image, analyzer-image) across all workflows and
wire them to the correct step/job outputs.
| waitFor() { | ||
| local timeout="$1" | ||
| shift | ||
| local what="$1" | ||
| shift | ||
| local cmd=("$@") | ||
|
|
||
| step "Waiting for ${what} (timeout ${timeout}s)..." | ||
| for ((i=1; i<=timeout; i+=5)); do | ||
| if "${cmd[@]}" &>/dev/null; then | ||
| success "${what} is ready" | ||
| return 0 | ||
| fi | ||
| sleep 5 | ||
| printf "." | ||
| done | ||
| die "Timeout waiting for ${what}" | ||
| } |
There was a problem hiding this comment.
waitFor doesn't enforce a wall-clock timeout.
The loop only increments i by 5, but callers already block inside kubectl wait --timeout=.... For example, Line 266 and Line 274 each wait up to 60s per probe, so waitFor 180 can stretch to ~39 minutes and hit the job timeout long before this helper trips.
⏱️ One way to enforce the real timeout budget
waitFor() {
local timeout="$1"
shift
local what="$1"
shift
local cmd=("$@")
+ local start=$SECONDS
step "Waiting for ${what} (timeout ${timeout}s)..."
- for ((i=1; i<=timeout; i+=5)); do
+ while (( SECONDS - start < timeout )); do
if "${cmd[@]}" &>/dev/null; then
success "${what} is ready"
return 0
fi
sleep 5
printf "."
done
die "Timeout waiting for ${what}"
}Keep the callback itself non-blocking, or derive its per-attempt timeout from the remaining budget.
📝 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.
| waitFor() { | |
| local timeout="$1" | |
| shift | |
| local what="$1" | |
| shift | |
| local cmd=("$@") | |
| step "Waiting for ${what} (timeout ${timeout}s)..." | |
| for ((i=1; i<=timeout; i+=5)); do | |
| if "${cmd[@]}" &>/dev/null; then | |
| success "${what} is ready" | |
| return 0 | |
| fi | |
| sleep 5 | |
| printf "." | |
| done | |
| die "Timeout waiting for ${what}" | |
| } | |
| waitFor() { | |
| local timeout="$1" | |
| shift | |
| local what="$1" | |
| shift | |
| local cmd=("$@") | |
| local start=$SECONDS | |
| step "Waiting for ${what} (timeout ${timeout}s)..." | |
| while (( SECONDS - start < timeout )); do | |
| if "${cmd[@]}" &>/dev/null; then | |
| success "${what} is ready" | |
| return 0 | |
| fi | |
| sleep 5 | |
| printf "." | |
| done | |
| die "Timeout waiting for ${what}" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tackle.sh` around lines 67 - 84, The waitFor helper (function
waitFor) currently increments i by fixed 5s and doesn't enforce a real
wall-clock budget because callers (e.g., kubectl wait --timeout=...) can block
per-attempt; change waitFor to track a start time and compute remaining_seconds
each loop iteration, then run the probe command with that per-attempt timeout
(e.g., wrap the probe in timeout or adjust kubectl --timeout) so no single probe
can exceed the remaining budget, break when remaining_seconds <= 0 and die with
timeout; ensure sleep uses min(5, remaining_seconds) and keep the
success/failure logic in waitFor unchanged.
| success "Tackle is ready!" | ||
|
|
||
| startPortForward |
There was a problem hiding this comment.
Don't block install on a foreground port-forward.
startPortForward never returns until Ctrl+C, so the workflow cannot reach the later status or uninstall steps. Make this opt-in, or skip it automatically in non-interactive sessions.
🚦 Minimal non-interactive guard
success "Tackle is ready!"
- startPortForward
+ if [[ -t 1 ]]; then
+ startPortForward
+ else
+ info "Skipping port-forward in non-interactive mode"
+ 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.
| success "Tackle is ready!" | |
| startPortForward | |
| success "Tackle is ready!" | |
| if [[ -t 1 ]]; then | |
| startPortForward | |
| else | |
| info "Skipping port-forward in non-interactive mode" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tackle.sh` around lines 574 - 576, The script currently calls
startPortForward (which runs foreground and blocks) unconditionally after
install, preventing later steps like status or uninstall from running; change
this so startPortForward is either opt-in or skipped in non-interactive sessions
by guarding the call with an interactive check or feature flag (e.g., only call
startPortForward when a PORT_FORWARD=true flag/option is set or when stdout is a
TTY via an interactive test like [ -t 1 ]), and ensure any CLI/install path or
CI environment skips the foreground port-forward so the workflow can continue to
status/uninstall.
| install) shift; cmdInstall "$@" ;; | ||
| status) cmdStatus ;; | ||
| uninstall) shift; cmdUninstall "$@" ;; |
There was a problem hiding this comment.
Only install honors --cluster.
cmdInstall parses --cluster, but Line 680 discards extra args for status, and cmdUninstall never parses the ones forwarded on Line 681. After install --cluster=foo, the rest of the CLI still points at tackle-test.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/tackle.sh` around lines 679 - 681, The case handling discards/doesn't
forward CLI args so only cmdInstall sees --cluster; update the case branches so
cmdStatus and cmdUninstall receive the original positional/flag args (and
cmdInstall still gets them) by forwarding "$@" to them (and removing any
unnecessary shift that drops flags) so that cmdStatus and cmdUninstall can parse
--cluster like cmdInstall; refer to the case labels and functions cmdInstall,
cmdStatus, and cmdUninstall when making the change.
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
Looks really good, the only thing that I can think is "missing" is the loading of the images into the kind cluster and downloading them from the artifacts. Other then that this is should be good to go.
| success "Keycloak configured. Admin user ready (user: ${adminUser}, pass: ${adminPass})" | ||
| } | ||
|
|
||
| startPortForward() { |
There was a problem hiding this comment.
Might be worth removing this because everything should be accessable via the already created kind networking config
|
|
||
| runs: | ||
| using: 'composite' | ||
| steps: |
There was a problem hiding this comment.
You are missing I think a step around downloading and setting the images up. and example is:
https://github.com/konveyor/ci/blob/main/koncur-tackle-hub/check_images.sh
and
load_img.sh
Might be worth moving these out of that action and into this action, if the koncur-tackle actions is going to use this action.
I use them here: to get the images into the kind cluster: https://github.com/konveyor/ci/blob/main/koncur-tackle-hub/action.yml#L66-L71
Hope that helps
2 participants
The existing workflows moved to saved-workflows are temporary and in tended to isolate the test-tackle workflow for now. They will be restored before this is merged.
Summary by CodeRabbit