👻 adding koncur tests to the demo testing,#1112
Open
shawn-hurley wants to merge 1 commit intokonveyor:mainfrom
Open
👻 adding koncur tests to the demo testing,#1112shawn-hurley wants to merge 1 commit intokonveyor:mainfrom
shawn-hurley wants to merge 1 commit intokonveyor:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,220,255,0.5)
participant Dev as Developer (push/PR)
participant GH as GitHub Actions
participant Detect as detect-changes job
participant ExtBuild as External Image Build Workflow
participant Registry as Artifact Registry
participant Tests as provider-tests / koncur jobs
end
Dev->>GH: push / open PR
GH->>Detect: run detect-changes (no image_tag output)
Detect-->>GH: pipeline proceeds using github.run_id
GH->>ExtBuild: trigger external build-images workflow with run_id
ExtBuild->>Registry: build & publish images tagged with run_id
Registry-->>GH: artifacts/images available
GH->>Tests: start provider-tests & koncur jobs
Tests->>Registry: download artifacts by run_id
Tests->>Tests: load images (podman), set IMG_* env vars
Tests-->>GH: run tests and report results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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.
Actionable comments posted: 4
🤖 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/demo-testing.yml:
- Around line 274-299: The matrix currently defines a macOS runner then
immediately excludes it—remove the macOS matrix entry (the object with os:
"macos" runner: "macos-26-large") or comment it out to avoid the contradictory
include+exclude in the koncur-kantra matrix; also address the undefined
inputs.ref in the action input ref expression (ref: ${{ github.base_ref ||
inputs.ref || github.ref_name }}) by either adding a workflow_dispatch input
named "ref" to the workflow inputs or by removing the inputs.ref alternative
from that expression so it only uses github.base_ref or github.ref_name.
- Around line 302-308: Fix the typo in the koncur-tackle-hub step's
image_pattern by renaming "tackle-keyloak-init" to "tackle-keycloak-init", and
also fix the ref fallback expression in the same step (the ref attribute) so it
uses the correct workflow input variable name (replace the incorrect inputs.ref
usage with the actual input name your workflow defines) to ensure the action
picks up the intended ref.
- Line 304: The reusable workflow reference on the `uses:` line is malformed;
update the string `shawn-hurley/ci/koncur-tackle-hub@main` to the GitHub Actions
reusable workflow format `OWNER/REPO/.github/workflows/FILENAME.yml@REF` (e.g.,
change to `shawn-hurley/ci/.github/workflows/koncur-tackle-hub.yml@main` or the
actual filename with .yaml) so the `uses:` invocation resolves correctly.
- Around line 42-46: The job block named build-images uses invalid keys: remove
the job-level id field (the job is already identified by the build-images key)
and fix the reusable workflow reference in the uses field to include the full
path and a ref; replace uses: shawn-hurley/ci/e2e-image-build.yaml with the
reusable-workflow format owner/repo/.github/workflows/e2e-image-build.yaml@ref
(e.g., shawn-hurley/ci/.github/workflows/e2e-image-build.yaml@main) so GitHub
Actions accepts the call.
shawn-hurley
force-pushed
the
prepare-for-koncur
branch
2 times, most recently
from
c6fa5ae to
62e16ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/demo-testing.yml:
- Around line 119-127: In the "Get nightly if needed" step fix the shell syntax
in the gh run download command: locate the gh run download invocation that sets
WORKFLOW_RUN and remove the stray closing quote after --dir /tmp/images and
either complete the dangling pipeline or remove the trailing pipe so the command
ends with a proper redirection (e.g., keep 2>&1 if you want stderr merged)
rather than "2>&1 |"; ensure the WORKFLOW_RUN variable and
matrix.nightly_pattern interpolation remain unchanged.
In `@external-providers/java-external-provider/Dockerfile`:
- Around line 2-3: Fix the typo in the Docker ARG by renaming ARG
JAVA_BUDLE_IMAGE to ARG JAVA_BUNDLE_IMAGE and update the FROM line to reference
${JAVA_BUNDLE_IMAGE} so external --build-arg JAVA_BUNDLE_IMAGE=<...> is honored;
also update any CI/workflow usages (e.g., the --build-arg flag in
demo-testing.yml or e2e-image-build.yaml) to use JAVA_BUNDLE_IMAGE to keep
callers consistent.
---
Duplicate comments:
In @.github/workflows/demo-testing.yml:
- Around line 314-323: In the koncur-tackle-hub job update the image_pattern and
ref usage: fix the misspelled image name in image_pattern from
"tackle-keyloak-init" to "tackle-keycloak-init" so the keycloak-init image is
matched, and remove the undefined "inputs.ref ||" fallback in the ref input so
ref becomes "${{ github.base_ref || github.ref_name }}" (refer to the
koncur-tackle-hub job, image_pattern key, and ref key in the workflow).
- Around line 286-311: Remove the dead macOS matrix entry and the undefined
workflow input reference: edit the koncur-kantra job's matrix (symbol "matrix"
with "os" entries and "exclude") to delete or comment out the macos/os: "macos"
runner: "macos-26-large" row instead of adding then excluding it, and update the
step input "ref" under the "with" block (the expression referencing "inputs.ref
||") to remove "inputs.ref ||" so the ref becomes just the existing ${{
github.base_ref || github.ref_name }} expression; keep the job name
"koncur-kantra", the "uses: shawn-hurley/ci/koncur-kantra@main" step and the
"image_pattern" intact.
Comment on lines
+2
to
+3
| ARG JAVA_BUDLE_IMAGE=quay.io/konveyor/jdtls-server-base:${JAVA_BUNDLE_TAG} | ||
| FROM ${JAVA_BUDLE_IMAGE} as base |
There was a problem hiding this comment.
Typo in ARG name: JAVA_BUDLE_IMAGE → JAVA_BUNDLE_IMAGE
The ARG declaration and FROM reference are internally consistent so the Docker build succeeds, but any caller that passes --build-arg JAVA_BUNDLE_IMAGE=<custom-image> (correct spelling) will have that value silently ignored and the build will fall back to the default.
🛠️ Proposed fix
-ARG JAVA_BUDLE_IMAGE=quay.io/konveyor/jdtls-server-base:${JAVA_BUNDLE_TAG}
-FROM ${JAVA_BUDLE_IMAGE} as base
+ARG JAVA_BUNDLE_IMAGE=quay.io/konveyor/jdtls-server-base:${JAVA_BUNDLE_TAG}
+FROM ${JAVA_BUNDLE_IMAGE} as baseAlso update the corresponding --build-arg flag in .github/workflows/demo-testing.yml (or the external e2e-image-build.yaml workflow) to use JAVA_BUNDLE_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.
Suggested change
| ARG JAVA_BUDLE_IMAGE=quay.io/konveyor/jdtls-server-base:${JAVA_BUNDLE_TAG} | |
| FROM ${JAVA_BUDLE_IMAGE} as base | |
| ARG JAVA_BUNDLE_IMAGE=quay.io/konveyor/jdtls-server-base:${JAVA_BUNDLE_TAG} | |
| FROM ${JAVA_BUNDLE_IMAGE} as base |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@external-providers/java-external-provider/Dockerfile` around lines 2 - 3, Fix
the typo in the Docker ARG by renaming ARG JAVA_BUDLE_IMAGE to ARG
JAVA_BUNDLE_IMAGE and update the FROM line to reference ${JAVA_BUNDLE_IMAGE} so
external --build-arg JAVA_BUNDLE_IMAGE=<...> is honored; also update any
CI/workflow usages (e.g., the --build-arg flag in demo-testing.yml or
e2e-image-build.yaml) to use JAVA_BUNDLE_IMAGE to keep callers consistent.
85dd4cc to
fb483d2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/demo-testing.yml:
- Around line 158-161: The conditional that checks the yq provider image using
yq_provider_image_regex mistakenly writes the image to IMG_GENERIC_PROVIDER in
the GITHUB_ENV; update the block guarded by [[ "$image" =~
$yq_provider_image_regex ]] to export/write IMG_YQ_PROVIDER=$image (and keep the
echo that logs the found image) instead of writing IMG_GENERIC_PROVIDER, so the
yq provider env var is correctly set and does not overwrite the generic
provider.
- Around line 131-139: Fix three bash syntax errors: close the unterminated
string on yq_provider_image_regex (add the missing closing double-quote for
yq_provider_image_regex), convert the invalid assignment
c_sharp_provider_image_regex = ".*c-sharp-provider.*" into a proper bash
assignment with no spaces around the equals sign
(c_sharp_provider_image_regex="..."), and replace the bad substitution when
loading images so the podman+awk pipeline is captured via command substitution
$(podman load -i "${image}" | awk '{print $NF}') and assigned to image (i.e.,
use $(...) not ${...} and ensure parentheses match); these changes will allow
the regex vars and the for loop (find /tmp/images ...; image=...) to parse and
run correctly.
- Line 173: The workflow is using matrix.image_env (e.g., IMG_JAVA_PROVIDER)
directly in the podman run command which expands to the variable name, not the
loaded image reference; instead, change the podman run invocation to perform
bash indirect expansion at runtime so the variable name stored in
matrix.image_env is dereferenced to the actual image value written to GITHUB_ENV
(update the podman run line that currently references matrix.image_env to use
runtime indirection and ensure the shell sees the environment variable set by
the "Load images" step).
In `@CODERABBIT_TODO.md`:
- Around line 1-359: CODERABBIT_TODO.md is an internal PR-review tracking
document that must not be committed to the repo; remove the file from the branch
(delete CODERABBIT_TODO.md and commit the deletion or revert the commit that
added it) so it is not merged into main, and instead create proper GitHub
Issues/project board entries (or a single tracking issue linking the items) to
preserve the feedback; update the PR to remove the file and ensure future
commits do not re-add it.
---
Duplicate comments:
In @.github/workflows/demo-testing.yml:
- Around line 314-339: Remove the redundant macOS entry from the koncur-kantra
job matrix (the matrix.os array) and also delete the matching exclude block so
macOS is no longer added then excluded; then add a workflow input named ref
(e.g., under workflow_dispatch inputs) or replace references to inputs.ref in
the koncur-kantra step with a guaranteed value (for example use only
github.ref_name or github.base_ref) so the expression ref: ${{ github.base_ref
|| inputs.ref || github.ref_name }} no longer references an undefined
inputs.ref; update the koncur-test step input usage accordingly.
- Around line 119-127: The "Get nightly if needed" step contains bash syntax
errors in the run block: remove the stray double-quote after --dir /tmp/images
and drop the dangling pipe at the end of the gh run download command so the
command completes (i.e., change gh run download ... --dir /tmp/images" 2>&1 | to
gh run download ... --dir /tmp/images 2>&1); ensure the WORKFLOW_RUN variable
and matrix.nightly_pattern usage remain unchanged and the run: block is properly
closed.
- Around line 342-352: Fix two issues in the koncur-tackle-hub job: correct the
misspelled image name in the image_pattern from "tackle-keyloak-init" to
"tackle-keycloak-init" (job name: koncur-tackle-hub, key field: image_pattern)
and replace the undefined inputs.ref expression in the ref input with a valid
reference source such as ${{ github.base_ref || github.event.inputs.ref ||
github.ref_name }} (field: ref) so the workflow uses an existing value.
Comment on lines
+1
to
+359
| # CodeRabbit Feedback TODO List - PR #1033 | ||
|
|
||
| ## Summary | ||
| - **Total Remaining:** 62 issues | ||
| - **Critical:** 12 (17%) | ||
| - **Code Quality:** 8 (11%) | ||
| - **Best Practices:** 15 (21%) | ||
| - **Testing:** 7 (10%) | ||
| - **Performance:** 3 (4%) | ||
| - **Minor/Nit:** 9 (13%) | ||
| - **Completed:** 9 issues (typos, documentation, worker count) | ||
|
|
||
| --- | ||
|
|
||
| ## 🚨 Critical Issues (12) - HIGH PRIORITY | ||
|
|
||
| ### 1. Race condition on slice append | ||
| - **File:** `konveyor/analyzer.go:150-174` | ||
| - **Issue:** Multiple goroutines concurrently append to `providerInitErrors` without synchronization | ||
| - **Fix:** Use mutex or channel to synchronize access | ||
|
|
||
| ### 2. Missing error checks for `progress.New()` (3 locations) | ||
| - **Files:** | ||
| - `cmd/analyzer/main.go:83-99` | ||
| - `cmd/dep/main.go:79-81` | ||
| - `provider/server.go:93-101` | ||
| - **Issue:** Error return from `progress.New()` is not checked | ||
| - **Fix:** Add error checking and handle appropriately | ||
|
|
||
| ### 3. Nil pointer dereference risks - Progress parameter (4 locations) | ||
| - **Files:** | ||
| - `lsp/base_service_client/base_service_client.go:258-262` | ||
| - `external-providers/generic-external-provider/pkg/server_configurations/nodejs/service_client.go:126-132` | ||
| - `external-providers/generic-external-provider/pkg/server_configurations/yaml_language_server/service_client.go:106-116` | ||
| - `provider/grpc/provider.go:360-373` | ||
| - **Issue:** Progress parameter passed to `Subscribe()` without nil check | ||
| - **Fix:** Add nil check before calling `Subscribe()` | ||
|
|
||
| ### 4. Execution continues after critical failures | ||
| - **File:** `cmd/analyzer/main.go:123-131` | ||
| - **Issue:** `ParseRules()` and `ProviderStart()` failures don't halt execution | ||
| - **Fix:** Add proper error handling to exit on critical failures | ||
|
|
||
| ### 5. Panic recovery incorrectly unlocks mutex | ||
| - **File:** `progress/collector/throttled_collector.go:108-149` | ||
| - **Issue:** Defer block unlocks mutex that may not be held, causing second panic | ||
| - **Fix:** Only unlock if mutex was successfully locked | ||
|
|
||
| ### 6. File handle leak - Progress output file | ||
| - **File:** `cmd/analyzer/main.go:226-249` | ||
| - **Issue:** Progress output file created but never closed | ||
| - **Fix:** Add `defer file.Close()` | ||
|
|
||
| ### 7. Process resource leaks (zombie processes) | ||
| - **File:** `lsp/base_service_client/cmd_dialer.go:44-52, 75-78` | ||
| - **Issue:** `Cmd.Wait()` never called, causing zombie processes. Race condition in `Cmd.Start()` in goroutine | ||
| - **Fix:** Call `Cmd.Wait()` to reap process. Fix race condition. | ||
|
|
||
| ### 8. Progress swallowed error | ||
| - **File:** `external-providers/generic-external-provider/pkg/generic_external_provider/provider.go:79-85` | ||
| - **Issue:** Error from `progress.New()` returns nil instead of actual error | ||
| - **Fix:** Return the error properly | ||
|
|
||
| ### 9. Nil panic in Unsubscribe | ||
| - **File:** `progress/progress.go:183-188` | ||
| - **Issue:** Calling nil cancel function if collector wasn't subscribed | ||
| - **Fix:** Check if cancel function exists before calling | ||
|
|
||
| ### 10. Resource leak - no Unsubscribe | ||
| - **File:** `lsp/base_service_client/base_service_client.go:258-262` | ||
| - **Issue:** throttledReporter subscribed but never unsubscribed in `Stop()` | ||
| - **Fix:** Call `Unsubscribe()` in `Stop()` method | ||
|
|
||
| ### 11. Blocking sends can stall system | ||
| - **File:** `progress/progress.go:158-169, 201-210` | ||
| - **Issue:** Blocking sends to reporter/collector channels can cascade failures | ||
| - **Fix:** Use select with default case or increase buffer sizes | ||
|
|
||
| ### 12. Multiple error handling bugs | ||
| - **File:** `konveyor/util.go:23-30` | ||
| - **Issues:** | ||
| - `filepath.Abs` error overwritten by `os.Stat` | ||
| - `errors.Is` misused (should be `errors.As`) | ||
| - Redundant stat calls | ||
| - **Fix:** Refactor error handling logic | ||
|
|
||
| --- | ||
|
|
||
| ## 🧹 Code Quality Issues (8) | ||
|
|
||
| ### 1. Unused progress field | ||
| - **File:** `provider/internal/builtin/provider.go:57-66` | ||
| - **Issue:** `progress` field added to struct but never used | ||
| - **Fix:** Remove field or implement usage | ||
|
|
||
| ### 2. Debug field never used | ||
| - **File:** `external-providers/generic-external-provider/pkg/server_configurations/pylsp/service_client.go:19` | ||
| - **Issue:** Field `blah int` is leftover from testing | ||
| - **Fix:** Remove the field | ||
|
|
||
| ### 3. Constructor ignores progress parameter | ||
| - **File:** `external-providers/generic-external-provider/pkg/generic_external_provider/provider.go:39-63` | ||
| - **Issue:** Parameter passed but never assigned to struct | ||
| - **Fix:** Assign parameter or remove it | ||
|
|
||
| ### 4. Dead code - unreachable validation | ||
| - **File:** `konveyor/types.go:62-64` | ||
| - **Issue:** `validationErrors` check can never be true | ||
| - **Fix:** Remove unreachable code | ||
|
|
||
| ### 5. Duplicate builtin configs | ||
| - **File:** `konveyor/util.go:47-52` | ||
| - **Issue:** Builtin config appended inside loop instead of once after | ||
| - **Fix:** Move append outside loop | ||
|
|
||
| ### 6. Debug print "HALKSJHLJF" | ||
| - **File:** `external-providers/generic-external-provider/pkg/server_configurations/generic/service_client.go:96-98` | ||
| - **Issue:** Debug print statement left in code | ||
| - **Fix:** Remove debug print | ||
|
|
||
| ### 7. Debug print "here - reporting channel reporting" | ||
| - **File:** `progress/reporter/channel_reporter.go:139-142` | ||
| - **Issue:** Debug print statement left in code | ||
| - **Fix:** Remove debug print | ||
|
|
||
| ### 8. Commented-out debug code | ||
| - **File:** `lsp/base_service_client/base_service_client.go:417-419` | ||
| - **Issue:** Commented-out debug code | ||
| - **Fix:** Remove commented code | ||
|
|
||
| ### 9. Unused createOpenAPISchema function | ||
| - **File:** `cmd/analyzer/main.go:252-374` | ||
| - **Issue:** Function defined but never called | ||
| - **Fix:** Remove function or implement usage | ||
|
|
||
| --- | ||
|
|
||
| ## 🔧 Best Practices & Refactoring (15) | ||
|
|
||
| ### 1. Variable shadows package name (3 locations) | ||
| - **Files:** | ||
| - `konveyor/analyzer.go:51-55` (collector) | ||
| - `konveyor/types.go:60-61` (collector) | ||
| - `cmd/dep/main.go:79-81` (progress) | ||
| - **Issue:** Variable name shadows package name | ||
| - **Fix:** Rename variable | ||
|
|
||
| ### 2. Loop uses wrong variable | ||
| - **File:** `konveyor/analyzer.go:82-85` | ||
| - **Issue:** Iterates over `a.rulePaths` instead of `rulePaths` parameter | ||
| - **Fix:** Use correct variable | ||
|
|
||
| ### 3. WithReporters overwrites instead of appending | ||
| - **File:** `konveyor/options.go:264-271` | ||
| - **Issue:** Replaces existing reporters instead of appending | ||
| - **Fix:** Append to existing reporters | ||
|
|
||
| ### 4. Premature stream termination | ||
| - **File:** `provider/server.go:601-619` | ||
| - **Issue:** Returns on first non-prepare event instead of filtering | ||
| - **Fix:** Filter events instead of returning | ||
|
|
||
| ### 5. Empty Stage field may confuse consumers | ||
| - **File:** `provider/grpc/service_client.go:272-279` | ||
| - **Issue:** Stage field left empty in events | ||
| - **Fix:** Populate Stage field appropriately | ||
|
|
||
| ### 6. Context cancellation returns nil instead of error | ||
| - **File:** `lsp/base_service_client/base_service_client.go:417-419` | ||
| - **Issue:** Returns nil when context is cancelled instead of error | ||
| - **Fix:** Return context error | ||
|
|
||
| ### 7. Switch statement duplicates logic | ||
| - **File:** `external-providers/generic-external-provider/pkg/generic_external_provider/provider.go:99-110` | ||
| - **Issue:** Duplicate logic in switch cases | ||
| - **Fix:** Consolidate logic | ||
|
|
||
| ### 8. Incorrect error message | ||
| - **File:** `lsp/base_service_client/cmd_dialer.go:83-85` | ||
| - **Issue:** Error message says "cannot close" in Dial method | ||
| - **Fix:** Correct error message | ||
|
|
||
| ### 9. Potentially confusing error for no dependencies | ||
| - **File:** `konveyor/analyzer.go:368-370` | ||
| - **Issue:** Error message unclear when no dependencies found | ||
| - **Fix:** Improve error message clarity | ||
|
|
||
| ### 10. GetProviders treats empty filters as match-none | ||
| - **File:** `konveyor/analyzer_test.go:158-163` | ||
| - **Issue:** Empty filters should match all, not none | ||
| - **Fix:** Change behavior or document it | ||
|
|
||
| ### 11. Named return inconsistency | ||
| - **File:** `konveyor/options.go:115-123` | ||
| - **Issue:** Mixed use of named and unnamed returns | ||
| - **Fix:** Be consistent | ||
|
|
||
| ### 12. selectorError returns empty string | ||
| - **File:** `konveyor/options.go:52-66` | ||
| - **Issue:** Returns empty string when no errors | ||
| - **Fix:** Return meaningful message or check for empty before calling | ||
|
|
||
| ### 13. Error from GetDependencies ignored | ||
| - **File:** `cmd/analyzer/main.go:133-140` | ||
| - **Issue:** Error not checked | ||
| - **Fix:** Handle error appropriately | ||
|
|
||
| ### 14. Engine options lack validation | ||
| - **File:** `konveyor/options.go:273-296` | ||
| - **Issue:** Engine options don't validate inputs | ||
| - **Fix:** Add validation | ||
|
|
||
| ### 15. Path format inconsistency | ||
| - **File:** `external-providers/generic-external-provider/e2e-tests/nodejs-e2e/provider_settings.json:7, 24` | ||
| - **Issue:** Inconsistent path format | ||
| - **Fix:** Standardize path format | ||
|
|
||
| --- | ||
|
|
||
| ## 🧪 Testing Suggestions (7) | ||
|
|
||
| ### 1. Tests use time.Sleep for synchronization (4 locations) | ||
| - **Files:** | ||
| - `progress/progress_test.go:162-169` | ||
| - `progress/reporter/reporter_test.go:255-276, 322-339, 675-705` | ||
| - `progress/collector/collector_test.go:12-63` | ||
| - **Issue:** Unreliable timing in tests | ||
| - **Fix:** Use channels or sync primitives | ||
|
|
||
| ### 2. Mock test fixtures not thread-safe | ||
| - **File:** `konveyor/test_helpers.go:81-87` | ||
| - **Issue:** mockReporter needs mutex for concurrent access | ||
| - **Fix:** Add mutex | ||
|
|
||
| ### 3. Missing positive test cases | ||
| - **File:** `konveyor/analyzer_test.go:12-28` | ||
| - **Issue:** Only tests empty/nil cases | ||
| - **Fix:** Add positive test cases | ||
|
|
||
| ### 4. Test error handling inconsistency | ||
| - **File:** `konveyor/config_test.go:73-78, 103-106` | ||
| - **Issue:** Inconsistent error handling in tests | ||
| - **Fix:** Standardize approach | ||
|
|
||
| ### 5. Mock lacks configurability | ||
| - **File:** `konveyor/test_helpers.go:89-109` | ||
| - **Issue:** Always returns true, can't test negative cases | ||
| - **Fix:** Make mock configurable | ||
|
|
||
| ### 6. Missing nil capabilities test | ||
| - **File:** `konveyor/provider_test.go:36-40` | ||
| - **Issue:** No test for nil capabilities | ||
| - **Fix:** Add test case | ||
|
|
||
| ### 7. Python e2e test had empty output | ||
| - **File:** `external-providers/generic-external-provider/e2e-tests/python-e2e/demo-output.yaml:1` | ||
| - **Status:** Now fixed in latest commits | ||
| - **Action:** Verify fix is complete | ||
|
|
||
| --- | ||
|
|
||
| ## ⚡ Performance Concerns (3) | ||
|
|
||
| ### 1. Random IDs could collide | ||
| - **File:** `progress/collector/base_collector.go:35-40` | ||
| - **Issue:** Using random IDs, should use atomic counter | ||
| - **Fix:** Replace with atomic counter | ||
|
|
||
| ### 2. Hardcoded worker count | ||
| - **File:** `lsp/base_service_client/base_service_client.go:298-301` | ||
| - **Issue:** 5 workers hardcoded, should be configurable | ||
| - **Fix:** Make configurable via option | ||
|
|
||
| ### 3. Hardcoded buffer size | ||
| - **File:** `progress/reporter/channel_reporter.go:53-59` | ||
| - **Issue:** Buffer size of 100 could be configurable | ||
| - **Fix:** Add configuration option | ||
|
|
||
| --- | ||
|
|
||
| ## 🎨 Minor/Nit Issues (9) | ||
|
|
||
| ### 9. Location assignment logic | ||
| - **File:** `konveyor/util_test.go:144-150` | ||
| - **Issue:** Location assignment logic could be simplified | ||
| - **Fix:** Refactor for clarity | ||
|
|
||
| ### 10. OpenAPI spec writes empty data | ||
| - **File:** `cmd/analyzer/main.go:107-121` | ||
| - **Issue:** Writing empty/placeholder OpenAPI schema data | ||
| - **Fix:** Implement or remove the flag | ||
|
|
||
| ### 11. Marshaling error ignored | ||
| - **File:** `cmd/analyzer/main.go:150-152` | ||
| - **Issue:** YAML marshaling error ignored with `_` | ||
| - **Fix:** Check and handle error | ||
|
|
||
| ### 12. Selector syntax validation deferred | ||
| - **File:** `konveyor/options.go:125-148` | ||
| - **Issue:** Selector syntax only validated when used, not at config time | ||
| - **Fix:** Validate immediately in option function | ||
|
|
||
| ### 13. Empty error string handling | ||
| - **File:** `konveyor/options.go:52-66` | ||
| - **Issue:** Returns empty string when `len(s) == 0` | ||
| - **Fix:** Return meaningful message | ||
|
|
||
| ### 14. Nil progress/reporter validation | ||
| - **File:** `konveyor/options.go:255-271` | ||
| - **Issue:** Should validate progress/reporter options are not nil | ||
| - **Fix:** Add nil validation | ||
|
|
||
| ### 15. Missing reportMutex initialization | ||
| - **File:** `progress/collector/throttled_collector.go:85-92` | ||
| - **Issue:** reportMutex not explicitly initialized (relies on zero value) | ||
| - **Fix:** Explicitly initialize mutex | ||
|
|
||
| ### 16. Provider config inconsistency | ||
| - **File:** `external-providers/generic-external-provider/e2e-tests/python-e2e/provider_settings.json:1-26` | ||
| - **Issue:** Config structure inconsistent with other provider configs | ||
| - **Fix:** Standardize config structure | ||
|
|
||
| ### 17. Errors.Join consistency | ||
| - **File:** `external-providers/java-external-provider/pkg/java_external_provider/dependency/decompile.go:310-311` | ||
| - **Issue:** Mixed patterns of error aggregation | ||
| - **Fix:** Use consistent pattern | ||
|
|
||
| --- | ||
|
|
||
| ## ✅ Completed Issues (9) | ||
|
|
||
| 1. ✅ Trailing slash inconsistency - `provider_pod_local_settings.json:163` | ||
| 2. ✅ Grammar improvement - `docs/konveyor-package.md:624-626` | ||
| 3. ✅ CodeSnipLimit description incorrect - `konveyor/config.go:80` | ||
| 4. ✅ Write errors ignored - `progress/reporter/json_reporter.go` and `text_reporter.go` | ||
| 5. ✅ Missing nil writer validation - `progress/reporter/json_reporter.go:60-64` and `text_reporter.go` | ||
| 6. ✅ Silent event drop in mock - `progress/progress_test.go:38-42` (added comment) | ||
| 7. ✅ Common update logic duplication - `progress/reporter/progress_bar_reporter.go:154-209` (added TODO comment) | ||
| 8. ✅ Typos fixed: "Staring" → "Starting", "collecterCancelMap" → "collectorCancelMap", "reporets" → "reporters" | ||
| 9. ✅ Worker count configurable - `konveyor/types.go:108-116` (implemented `WithWorkerCount()` option) | ||
|
|
||
| --- | ||
|
|
||
| ## Recommended Priority Order | ||
|
|
||
| 1. **Critical Issues (12)** - Address immediately to prevent bugs, crashes, resource leaks | ||
| 2. **Code Quality (8)** - Quick wins, remove dead/debug code | ||
| 3. **Best Practices (15)** - Improve maintainability | ||
| 4. **Minor/Nit (9)** - Low-hanging fruit | ||
| 5. **Performance (3)** - Make things configurable | ||
| 6. **Testing (7)** - Improve test reliability | ||
|
|
||
| --- | ||
|
|
||
| ## Notes | ||
|
|
||
| - This list was generated from CodeRabbit feedback on PR #1033 | ||
| - PR link: https://github.com/konveyor/analyzer-lsp/pull/1033 | ||
| - Date categorized: 2026-02-17 |
There was a problem hiding this comment.
This internal tracking document does not belong in the repository.
CODERABBIT_TODO.md is a review-feedback tracking list scoped entirely to PR #1033 (a separate, unrelated PR). Committing it to the main branch:
- Conflates repository documentation with ephemeral PR-review artefacts.
- Will permanently pollute git history with review notes that have no bearing on project functionality.
- Is inconsistent with the PR's stated objective ("adding koncur tests to the demo testing").
Track these items in GitHub Issues (one issue per item, or a single tracking issue), a project board, or a private note — not as a committed file.
🧰 Tools
🪛 LanguageTool
[grammar] ~79-~79: Use a hyphen to join words.
Context: ...ase buffer sizes ### 12. Multiple error handling bugs - File: `konveyor/util...
(QB_NEW_EN_HYPHEN)
[grammar] ~111-~111: Ensure spelling is correct
Context: ...move unreachable code ### 5. Duplicate builtin configs - File: `konveyor/util.go:4...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~113-~113: Ensure spelling is correct
Context: ...* konveyor/util.go:47-52 - Issue: Builtin config appended inside loop instead of ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CODERABBIT_TODO.md` around lines 1 - 359, CODERABBIT_TODO.md is an internal
PR-review tracking document that must not be committed to the repo; remove the
file from the branch (delete CODERABBIT_TODO.md and commit the deletion or
revert the commit that added it) so it is not merged into main, and instead
create proper GitHub Issues/project board entries (or a single tracking issue
linking the items) to preserve the feedback; update the PR to remove the file
and ensure future commits do not re-add it.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/demo-testing.yml:
- Around line 141-160: The log messages and echo statements in the
image-detection blocks are inconsistent and misleading: when matching
c_sharp_provider_image_regex you print "Java Provider Image Found" and when
matching yq_provider_image_regex you print "Generic Provider Image Found", and
some log text uses JAVA_PROVIDER_IMG while the actual env var is
IMG_JAVA_PROVIDER. Update the echo messages to correctly describe each block
(use "C-Sharp Provider Image Found" for c_sharp_provider_image_regex, "YQ
Provider Image Found" for yq_provider_image_regex, and "Generic Provider Image
Found" for generic_provider_image_regex) and make the log text mirror the env
var names you append to GITHUB_ENV (use IMG_JAVA_PROVIDER, IMG_C_SHARP_PROVIDER,
IMG_ANALYZER, IMG_GENERIC_PROVIDER, IMG_YQ_PROVIDER consistently with the echo
lines) so logs and env writes match the variables java_provider_image_regex,
c_sharp_provider_image_regex, analyzer_regex, generic_provider_image_regex, and
yq_provider_image_regex.
---
Duplicate comments:
In @.github/workflows/demo-testing.yml:
- Around line 124-127: The shell step that sets WORKFLOW_RUN and runs gh run
download has a stray double-quote at the end of the --dir argument and an
unmatched trailing pipe, which breaks the bash script; update the run block so
the gh run download invocation (referencing WORKFLOW_RUN and
matrix.nightly_pattern) removes the extra '"' after /tmp/images and either
completes or removes the trailing '|' so the pipeline is valid (or redirect 2>&1
to the intended consumer), ensuring the command is a single well-formed
pipeline.
- Around line 131-136: Fix the two bash syntax errors: close the missing quote
on the variable yq_provider_image_regex
(yq_provider_image_regex=".*yq-external-provider.*") and remove spaces around
the equals in c_sharp_provider_image_regex so it becomes a valid assignment
(c_sharp_provider_image_regex=".*c-sharp-provider.*"); update the lines that
define yq_provider_image_regex and c_sharp_provider_image_regex accordingly.
- Around line 313-338: The koncur-kantra job currently defines a macOS matrix
entry but then excludes it and also references a non-existent workflow input
`inputs.ref`; update the matrix to remove the macOS entry and its corresponding
exclude block (remove the macos/macos-26-large entry and the exclude section),
and simplify the `ref` input for the action (in the Run Koncur Testing step) to
use only defined refs, e.g. replace `${{ github.base_ref || inputs.ref ||
github.ref_name }}` with `${{ github.base_ref || github.ref_name }}` so it no
longer relies on the undefined `inputs.ref`.
- Around line 341-351: Update the koncur-tackle-hub job: fix the typo in the
image_pattern glob from "tackle-keyloak-init" to "tackle-keycloak-init" and
remove the undefined fallback "inputs.ref" in the ref expression; instead use a
defined fallback like "github.base_ref || github.ref_name" (so the ref line
becomes ref: ${{ github.base_ref || github.ref_name }}). This touches the
koncur-tackle-hub job's image_pattern string and the ref expression to ensure
the keycloak image is matched and the ref fallback is valid.
- Line 172: The workflow currently uses ${{ matrix.image_env }} in the podman
run command which expands to the environment variable name (e.g.
IMG_JAVA_PROVIDER) at workflow parse time instead of its value; fix by
performing bash indirect expansion at runtime: capture the matrix value into a
shell var (e.g. IMAGE_VAR="${{ matrix.image_env }}"), then resolve the actual
image with indirect expansion (e.g. IMAGE="${!IMAGE_VAR}:${{ github.run_id }}")
and pass that IMAGE into the podman run invocation. Update the podman run step
to use the resolved IMAGE variable instead of ${{ matrix.image_env }}:${{
github.run_id }}.
65a55ea to
7278a79
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/demo-testing.yml:
- Around line 42-47: The reusable workflow invocation "build-images" (uses:
shawn-hurley/ci/.github/workflows/e2e-image-build.yaml@main) currently relies on
inherited secrets which aren't supported for external repos; update the job to
explicitly pass required secrets (e.g., add a "secrets:" block mapping needed
names like REGISTRY_TOKEN: ${{ secrets.REGISTRY_TOKEN }} or other registry
credentials) and confirm which secret names the external workflow expects before
adding them.
---
Duplicate comments:
In @.github/workflows/demo-testing.yml:
- Around line 313-338: The matrix adds a macOS entry then immediately excludes
it and the run step's ref uses an undefined inputs.ref; remove the macOS matrix
entry and the entire exclude block (the matrix.os entry with os: "macos" and
runner: "macos-26-large" and the matrix.exclude block) so the job matrix only
contains valid runners, and update the Run Koncur Testing step's ref input (in
the step with id koncur-test that uses shawn-hurley/ci/koncur-kantra@main) to
remove the unused inputs.ref fallback, e.g. use ref: ${{ github.base_ref ||
github.ref_name }}.
- Around line 119-126: In the "Get nightly if needed" run step, fix the shell
syntax in the gh run download invocation (the WORKFLOW_RUN and gh run download
lines): remove the stray closing double-quote after --dir /tmp/images and remove
the trailing pipe at the end of the line (or replace it with a valid consumer
such as piping to tee or redirecting output); also ensure the --pattern argument
is properly quoted (e.g., wrap the pattern in single quotes) so the shell won't
misinterpret glob characters.
- Around line 141-160: Fix the copy-paste mismatches in the image-detection
blocks so logged messages match the actual env var written: in the block guarded
by java_provider_image_regex, change the echo to reference IMG_JAVA_PROVIDER
(not JAVA_PROVIDER_IMG); in the c_sharp_provider_image_regex block, change the
echo message to "C# Provider Image Found" and ensure it references
IMG_C_SHARP_PROVIDER; in the generic_provider_image_regex block, change the echo
to reference IMG_GENERIC_PROVIDER (not GENERIC_PROVIDER_IMG); and in the
yq_provider_image_regex block, change the echo message to "YQ Provider Image
Found" and ensure it references IMG_YQ_PROVIDER so each echo and the >>
$GITHUB_ENV line are consistent with the regex variable names
(java_provider_image_regex, c_sharp_provider_image_regex, analyzer_regex,
generic_provider_image_regex, yq_provider_image_regex) and the intended env vars
(IMG_JAVA_PROVIDER, IMG_C_SHARP_PROVIDER, IMG_ANALYZER, IMG_GENERIC_PROVIDER,
IMG_YQ_PROVIDER).
- Around line 341-350: Fix two issues in the koncur-tackle-hub job: correct the
image_pattern glob name from "tackle-keyloak-init" to "tackle-keycloak-init" so
the keycloak-init image is matched, and remove the undefined inputs.ref from the
ref expression (replace the interpolation "${{ github.base_ref || inputs.ref ||
github.ref_name }}" with "${{ github.base_ref || github.ref_name }}" or with the
actual action input name if you intended a different input); update the job's
ref argument (symbol: ref) accordingly.
In `@CODERABBIT_TODO.md`:
- Around line 1-359: The file CODERABBIT_TODO.md is an internal PR review
tracking doc that must not be committed to the repo; remove the file from the
branch (delete CODERABBIT_TODO.md and amend the commit or create a new commit
that deletes it), move the actionable items into the appropriate external
tracker (create GitHub Issues or a project board referencing PR `#1033` and any
unique entries you want preserved), and ensure no follow-up commits re-add the
file (verify with git status/branch diff before push); reference
CODERABBIT_TODO.md and PR `#1033` when creating the issues so the review context
is preserved.
Comment on lines
+42
to
+47
| build-images: | ||
| name: Build e2e Images | ||
| uses: shawn-hurley/ci/.github/workflows/e2e-image-build.yaml@main | ||
| with: | ||
| repo: ${{ github.repository }} | ||
| ref: ${{ github.ref }} |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions reusable workflow secrets: inherit external repository
💡 Result:
secrets: inherit only “inherits” secrets from the caller repository into the called reusable workflow, and GitHub limits its use to callers that reuse workflows within the same organization or enterprise. If the reusable workflow lives in an external repository (outside your org/enterprise), you generally must pass secrets explicitly. [1]
What to do when the reusable workflow is in another repo
Caller workflow (external repo reference): pass secrets by name
jobs:
use-reusable:
uses: other-owner/other-repo/.github/workflows/reusable.yml@v1
secrets:
DEPLOY_TOKEN: ${{ secrets.DEPLOY_TOKEN }}
SLACK_WEBHOOK: ${{ secrets.SLACK_WEBHOOK }}In this model, the secrets still come from the caller repo/org, not the called repo. GitHub does not support “central secrets” living only in the reusable-workflow repo and automatically flowing outward. [1] [2]
Reusable workflow: declare the secrets it expects
on:
workflow_call:
secrets:
DEPLOY_TOKEN:
required: true
SLACK_WEBHOOK:
required: trueThen use them as secrets.DEPLOY_TOKEN, etc. [1]
Notes / common gotchas
- Secrets are not automatically passed to reusable workflows—you must either map them explicitly (works anywhere you can call the workflow) or use
secrets: inheritwhere allowed. [3] [1] - Environment secrets can’t be passed via
workflow_call(andenvironment:in the called workflow uses that environment’s secrets instead). [1] - If the reusable workflow repo is private, you must configure it to be accessible to other repos under Repo Settings → Actions → General → Access (otherwise the caller can’t download the workflow). [4]
Clarify how secrets are passed to this external reusable workflow.
For reusable workflows in external repositories (different owner), secrets: inherit is not supported by GitHub Actions—it only works for workflows within the same organization or enterprise. If the external workflow (shawn-hurley/ci/.github/workflows/e2e-image-build.yaml) needs registry credentials to push images, you must pass them explicitly by name:
with:
repo: ${{ github.repository }}
ref: ${{ github.ref }}
secrets:
REGISTRY_TOKEN: ${{ secrets.REGISTRY_TOKEN }}Verify whether the external workflow requires any secrets and, if so, pass them explicitly using the pattern above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/demo-testing.yml around lines 42 - 47, The reusable
workflow invocation "build-images" (uses:
shawn-hurley/ci/.github/workflows/e2e-image-build.yaml@main) currently relies on
inherited secrets which aren't supported for external repos; update the job to
explicitly pass required secrets (e.g., add a "secrets:" block mapping needed
names like REGISTRY_TOKEN: ${{ secrets.REGISTRY_TOKEN }} or other registry
credentials) and confirm which secret names the external workflow expects before
adding them.
1aa60b0 to
f78d4d1
Compare
…orkflow image builds
shawn-hurley
force-pushed
the
prepare-for-koncur
branch
from
f78d4d1 to
bab2c22
Compare
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
1 participant
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.
prepare for koncur
Summary by CodeRabbit
Chores
New Features
Documentation