MTV-2947 | Add an cli download image#2882
Conversation
yaacov
requested review from
Hazanel,
mnecas,
mrnold and
solenoci
as code owners
|
Warning Rate limit exceeded@yaacov has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 0 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (22)
WalkthroughAdds a CLI download service: new forklift-cli-download image and artifacts, Makefile build/push targets and build-arg, download script, operator wiring (env var, RBAC), CRD additions for CLI download configuration and feature flag, Ansible templates/tasks for Service/Deployment/Route and ConsoleCLIDownloads, plus image export and CRD validation updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Admin
participant Operator as Forklift Operator
participant K8s as Kubernetes API
participant Console as OpenShift Console
participant CDN as CLI Download Pod (nginx)
Admin->>Operator: Apply ForkliftController (feature_cli_download="true")
Operator->>K8s: Create Service, Deployment, Route (cli-download)
Operator->>K8s: Create ConsoleCLIDownload resources
K8s-->>Operator: Resources Ready
Note over CDN: nginx serves static kubectl-mtv & MCP artifacts on :8080
Console->>K8s: Read ConsoleCLIDownload metadata (links)
Console-->>Admin: Render download links
Admin->>Console: Click download link
Console->>K8s: Route to Service
K8s->>CDN: Proxy request
CDN-->>Admin: Return artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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 |
yaacov
force-pushed
the
cli-download-service
branch
3 times, most recently
from
6cce422 to
2d199cc
Compare
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2882 +/- ##
=========================================
- Coverage 15.45% 8.27% -7.19%
=========================================
Files 112 381 +269
Lines 23377 52684 +29307
=========================================
+ Hits 3613 4359 +746
- Misses 19479 47980 +28501
- Partials 285 345 +60
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 30
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/validate_forklift_controller_crd.py (1)
67-76: Guard against non‑mapping defaults and keep behavior deterministic.safe_load may return None or non‑dict; protect before .keys().
- with open(defaults_file, 'r') as f: - defaults_data = yaml.safe_load(f) - - # Add variables from defaults that should correspond to CRD properties - defaults_variables = set(defaults_data.keys()) + with open(defaults_file, 'r') as f: + defaults_data = yaml.safe_load(f) or {} + if not isinstance(defaults_data, dict): + defaults_data = {} + # Add variables from defaults that should correspond to CRD properties + defaults_variables = set(defaults_data.keys())
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
build/forklift-cli-download/README.mdis excluded by!**/*.mdbuild/forklift-cli-download/artifacts/kubectl-mtv-darwin-amd64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-darwin-arm64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-linux-amd64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-linux-arm64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-windows-amd64.zipis excluded by!**/*.zip,!**/*.zip
📒 Files selected for processing (22)
Makefile(5 hunks)build/forklift-cli-download/Containerfile(1 hunks)build/forklift-cli-download/Containerfile-downstream(1 hunks)build/forklift-cli-download/download-latest-release.sh(1 hunks)build/forklift-operator-bundle/Containerfile(1 hunks)build/forklift-operator-bundle/Containerfile-downstream(1 hunks)hack/validate_forklift_controller_crd.py(2 hunks)operator/.downstream_manifests(10 hunks)operator/.kustomized_manifests(10 hunks)operator/.upstream_manifests(10 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(2 hunks)operator/config/manager/manager.yaml(1 hunks)operator/config/rbac/role.yaml(1 hunks)operator/export-vars-downstream.sh(2 hunks)operator/export-vars-upstream.sh(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(3 hunks)operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/route-cli-download.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/service-cli-download.yml.j2(1 hunks)
🧰 Additional context used
🧠 Learnings (8)
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-16T12:04:39.609Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:1253-1270
Timestamp: 2025-09-16T12:04:39.609Z
Learning: The files operator/.kustomized_manifests and operator/.upstream_manifests are also auto-generated manifests in the Forklift project, similar to operator/.downstream_manifests.
Applied to files:
operator/.downstream_manifestsoperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-16T12:04:53.070Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:4344-4361
Timestamp: 2025-09-16T12:04:53.070Z
Learning: The file `operator/.downstream_manifests` in the Forklift project is auto-generated and should not be edited directly. Any formatting issues or changes need to be addressed in the source templates or generation logic that creates this file.
Applied to files:
operator/.downstream_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-16T12:04:39.609Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:1253-1270
Timestamp: 2025-09-16T12:04:39.609Z
Learning: The file operator/.downstream_manifests is auto-generated and cannot be directly modified. Any formatting fixes need to be applied to the source templates or generation process that creates this file.
Applied to files:
operator/.downstream_manifests
📚 Learning: 2025-09-16T12:06:30.507Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/config/crd/bases/forklift.konveyor.io_migrations.yaml:547-565
Timestamp: 2025-09-16T12:06:30.507Z
Learning: The file operator/config/crd/bases/forklift.konveyor.io_migrations.yaml is auto-generated and cannot be directly modified. Any formatting fixes need to be applied to the source templates or generation process that creates this file.
Applied to files:
operator/.downstream_manifestsoperator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-09-03T04:08:05.745Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.downstream_manifests:2523-2534
Timestamp: 2025-09-03T04:08:05.745Z
Learning: For Forklift Plan CRD docs, maintainer prefers documenting existing runtime keys/behavior verbatim (e.g., TargetLabels uses "app", convertor pods use "forklift.app") and avoiding normalization changes in documentation-only PRs.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-09-17T03:50:40.352Z
Learnt from: mansam
PR: kubev2v/forklift#2855
File: operator/roles/forkliftcontroller/defaults/main.yml:144-144
Timestamp: 2025-09-17T03:50:40.352Z
Learning: In operator/roles/forkliftcontroller/defaults/main.yml, hooks_container_requests_memory (for hooks containers) and ova_container_requests_memory (for OVA provider server containers) are separate components with different memory requirements and should not be changed together.
Applied to files:
operator/roles/forkliftcontroller/defaults/main.yml
🪛 Checkov (3.2.334)
operator/roles/forkliftcontroller/tasks/main.yml
[low] 241-270: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / forklift-operator-dev-preview-on-pull-request
- GitHub Check: Red Hat Konflux / forklift-operator-bundle-dev-preview-on-pull-request
- GitHub Check: build
🔇 Additional comments (25)
build/forklift-cli-download/Containerfile-downstream (1)
1-4: Static nginx setup looks good.Base image, artifacts path, port, and foreground CMD align with the Service at 8080.
operator/roles/forkliftcontroller/templates/cli-download/service-cli-download.yml.j2 (1)
10-20: Ensure selector labels match the Deployment.Selector uses app and service labels; verify the Deployment template sets both identically, or the Service will not target pods. You can drop sessionAffinity: None (default) if you want less noise.
build/forklift-cli-download/download-latest-release.sh (1)
24-35: Prefer curl over wget for consistency and fewer build deps.Also surface errors when a download fails.
[ suggest_recommended_refactor ]- echo "Downloading $filename..." - if wget -q --show-progress -O "${ARTIFACTS_DIR}/${filename}" "${BASE_URL}/${source_name}"; then - echo "✓ $filename" - else - echo "✗ Failed: $filename" - fi + echo "Downloading $filename..." + if curl -fSL -o "${ARTIFACTS_DIR}/${filename}" "${BASE_URL}/${source_name}"; then + echo "✓ $filename" + else + echo "✗ Failed: $filename" >&2 + return 1 + fioperator/roles/forkliftcontroller/defaults/main.yml (2)
102-112: Image/env wiring looks consistent; keep it env‑driven.Good use of RELATED_IMAGE fallback and modest resource requests/limits.
12-12: Revisit default enablement of DEV PREVIEW feature_cli_downloadfeature_cli_download is true by default (operator/roles/forkliftcontroller/defaults/main.yml:12) while the CLI artifacts are marked "DEV PREVIEW" (operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2, console-cli-download-mcp.yml.j2); when enabled the role sets cli_download_state to present and creates the CLI service/deployment (operator/roles/forkliftcontroller/tasks/main.yml). Default to false or gate via release channel/ENV to avoid surprising upgrades. Do product requirements mandate this be enabled by default in all environments?
operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2 (1)
12-14: Make download link absolute using the Route hostFile: operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2 Lines: 12-14
- Replace relative href with the Route host:
- - href: "/kubectl-mtv-mcp-servers-linux-amd64.tar.gz" + - href: "https://{{ cli_download_route_host }}/kubectl-mtv-mcp-servers-linux-amd64.tar.gz"
- Do we also need arm64 MCP servers? If not available, add a TODO or include the arm64 artifact.
operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2 (1)
12-22: Use absolute Route URLs; capture the Route host into cli_download_route_host firstConsoleCLIDownload currently uses leading "/" links (these point to the Console host). I found no cli_download_route_host variable or task that registers the Route host — add a task (after creating the Route in operator/roles/forkliftcontroller/tasks/main.yml) to set cli_download_route_host from the Route's host, then update the template links to use that host:
- - href: "/kubectl-mtv-linux-amd64.tar.gz" + - href: "https://{{ cli_download_route_host }}/kubectl-mtv-linux-amd64.tar.gz" - - href: "/kubectl-mtv-linux-arm64.tar.gz" + - href: "https://{{ cli_download_route_host }}/kubectl-mtv-linux-arm64.tar.gz" - - href: "/kubectl-mtv-darwin-amd64.tar.gz" + - href: "https://{{ cli_download_route_host }}/kubectl-mtv-darwin-amd64.tar.gz" - - href: "/kubectl-mtv-darwin-arm64.tar.gz" + - href: "https://{{ cli_download_route_host }}/kubectl-mtv-darwin-arm64.tar.gz" - - href: "/kubectl-mtv-windows-amd64.zip" + - href: "https://{{ cli_download_route_host }}/kubectl-mtv-windows-amd64.zip"operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2 — update links; operator/roles/forkliftcontroller/tasks/main.yml — add task to capture Route host.
operator/.upstream_manifests (3)
61-63: api_image_fqin schema tightened — LGTMExample updated to kubev2v and explicit type: string. Matches other image fields.
269-269: Image example repo switches to quay.io/kubev2v — LGTMExamples align with current registry naming.
Also applies to: 289-289, 293-293, 297-297, 301-301, 305-305, 325-325, 345-345, 353-353, 387-387
63-82: New CLI download config fields (limits/requests + image) — wiring and OpenShift gating verified
- Controller/Ansible consumes the new vars and has sane defaults: see operator/roles/forkliftcontroller/defaults/main.yml and operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 (image + limits/requests used).
- Fields are exposed in the CRD (operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml).
- Creation of Route/ConsoleCLIDownload is OpenShift‑only: tasks use
when: feature_cli_download|bool and not k8s_cluster|booland console templates are console.openshift.io/v1 — see operator/roles/forkliftcontroller/tasks/main.yml and operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-*.yml.j2.build/forklift-cli-download/Containerfile (1)
2-2: Ensure artifacts are present at build time.Build will fail if build/forklift-cli-download/artifacts is missing. Confirm CI runs the download-latest-release.sh (or equivalent) before building.
Makefile (3)
89-89: Expose CLI_DOWNLOAD_IMAGE default — LGTMConsistent with other image defaults and enables override.
378-379: Include CLI image in build-all — ordering is correctBuilt before the bundle, so the bundle ARG resolves to the freshly built tag.
392-394: Include CLI image in push-all — LGTMPushing the CLI image before the bundle/index is the right order.
operator/roles/forkliftcontroller/tasks/main.yml (1)
9-13: Feature flag state for CLI download — LGTMSets cli_download_state only when enabled; downstream tasks are gated accordingly.
operator/.kustomized_manifests (4)
61-61: Image example updates to quay.io/kubev2v — LGTM.Consistent with current image publishing locations.
Also applies to: 117-117, 269-269, 289-289, 293-293, 297-297, 301-301, 305-305, 325-325, 345-345, 353-353, 387-387
63-82: CLI download CRD fields wired; CI coverage still requiredVerified: templates reference the new fields (operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 uses cli_download_image_fqin and cli_download_container_*; service/route/console templates exist in operator/roles/forkliftcontroller/templates/cli-download), defaults are set in operator/roles/forkliftcontroller/defaults/main.yml, CRD contains the new fields (operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml), and feature_cli_download gates tasks in operator/roles/forkliftcontroller/tasks/main.yml.
Action: add/update CI tests to validate CRD -> rendered manifests or reconciliation exercise these spec keys (manifest generation/reconcile checks).
6308-6311: Gate ConsoleCLIDownload RBAC on non‑OCP clustersRBAC for ConsoleCLIDownload (operator/.kustomized_manifests lines 6308–6311) is appropriate for OpenShift; ensure any tasks/roles that create ConsoleCLIDownload resources are skipped when the console.openshift.io API is absent (pure Kubernetes). Verification couldn't be completed: the searched paths (roles/, playbooks/) were not present in the sandbox (rg output: "roles/: No such file or directory"). If the role/playbook exists elsewhere, run from the repo root and paste results:
rg -nC3 -e 'ConsoleCLIDownload|route\.openshift\.io|console\.openshift\.io' -S || true rg -nC3 -e '\bk8s_cluster\b|\bfeature_cli_download\b' -S || true
179-184: No action required — operator already defaults feature_cli_download to true.operator/roles/forkliftcontroller/defaults/main.yml sets
feature_cli_download: trueand tasks reference it (operator/roles/forkliftcontroller/tasks/main.yml); adding the field to samples/CSV is optional for explicitness.operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (3)
187-204: CLI download resource knobs added — defaults present and units match.Defaults set to cli_download_container_limits_cpu=100m, cli_download_container_limits_memory=128Mi, cli_download_container_requests_cpu=50m, cli_download_container_requests_memory=64Mi; templates reference the same variables.
74-77: Feature gate schema OK — Ansible already converts the string to bool.CRD string enum matches project convention; tasks use feature_cli_download|bool (see operator/roles/forkliftcontroller/tasks/main.yml), so "false" won't be treated as truthy.
96-100: cli_download_image_fqin wiring verified – the CRD defines spec.cli_download_image_fqin, manager.yaml and both export-vars scripts expose CLI_DOWNLOAD_IMAGE, and the Ansible templates use the merged cli_download_image_fqin variable (from spec or env) for the image.operator/.downstream_manifests (3)
61-63: Image example updates to quay.io/kubev2v — LGTM.Consistent branding and examples across CRD fields look good.
Also applies to: 117-118, 269-270, 289-290, 293-294, 297-298, 301-302, 305-306, 325-326, 345-346, 353-354, 385-387
179-184: Gating for CLI download already implemented — no change required.The role tasks gate creation and cleanup: operator/roles/forkliftcontroller/tasks/main.yml contains "when: feature_cli_download|bool and not k8s_cluster|bool" (setup) and "when: not feature_cli_download|bool" (cleanup). CLI templates live under operator/roles/forkliftcontroller/templates/cli-download (route-cli-download.yml.j2, console-cli-download-*.yml.j2). Defaults set feature_cli_download in operator/roles/forkliftcontroller/defaults/main.yml.
63-82: CRD keys consumed and defaults present — no action required.
- CRD defines the fields: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (cli_download_container_* and cli_download_image_fqin present).
- Defaults exist in operator/roles/forkliftcontroller/defaults/main.yml (cli_download_container_limits_cpu: "100m", cli_download_container_limits_memory: "128Mi", cli_download_container_requests_cpu: "50m", cli_download_container_requests_memory: "64Mi"; cli_download_image_fqin resolved from env).
- Template uses them at operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 (image and resources mapped).
Optional: add lightweight regex validation in the generator (not the auto-generated manifests), e.g. ^\d+m$ for CPU and ^\d+(Mi|Gi)$ for memory.
yaacov
force-pushed
the
cli-download-service
branch
5 times, most recently
from
f8f3e23 to
8d8b4fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (20)
build/forklift-operator-bundle/Containerfile (1)
15-15: ARG added but redundant with export-vars; remove or make it effective.envsubst already receives CLI_DOWNLOAD_IMAGE from
export-vars-${STREAM}.sh(see Lines 24–27). As-is, this ARG won’t influence the generated bundle. Either drop it, or promote to ENV before the envsubst step if you want build-arg overrides to apply.Option A — remove:
-ARG CLI_DOWNLOAD_IMAGE="quay.io/kubev2v/forklift-cli-download:latest"Option B — make it take effect:
ARG UI_PLUGIN_IMAGE="quay.io/kubev2v/forklift-console-plugin:latest" -ARG CLI_DOWNLOAD_IMAGE="quay.io/kubev2v/forklift-cli-download:latest" +ARG CLI_DOWNLOAD_IMAGE="quay.io/kubev2v/forklift-cli-download:latest" +ENV CLI_DOWNLOAD_IMAGE="${CLI_DOWNLOAD_IMAGE}"hack/validate_forklift_controller_crd.py (1)
91-173: Hoist and dedupe exclusion set; remove overlap with ansible_builtin.excluded_fields repeats entries (e.g., app_namespace, forklift_resources, ui_plugin_console_name) and overlaps with ansible_builtin. Centralize as a module-level constant and reference it here.
One possible refactor:
@@ -import os import sys import yaml import argparse import re from pathlib import Path +from typing import FrozenSet @@ - # Filter out Ansible built-in variables, internal variables, and false positives + # Filter out Ansible built-in variables, internal variables, and false positives ansible_builtin = { @@ - # Also filter out the calculated/derived variables that shouldn't be in CRD - # (apply the same exclusions that were applied to defaults) - excluded_fields = { - 'app_name', - 'app_namespace', - 'forklift_operator_version', - 'forklift_resources', - # Service and component names - 'controller_configmap_name', - 'controller_service_name', - 'controller_deployment_name', - 'controller_container_name', - 'ovirt_osmap_configmap_name', - 'vsphere_osmap_configmap_name', - 'virt_customize_configmap_name', - 'profiler_volume_path', - # Inventory service fields - 'inventory_volume_path', - 'inventory_container_name', - 'inventory_service_name', - 'inventory_route_name', - 'inventory_route_timeout', - 'inventory_tls_secret_name', - 'inventory_issuer_name', - 'inventory_certificate_name', - # Services configuration - 'services_service_name', - 'services_route_name', - 'services_tls_secret_name', - 'services_issuer_name', - 'services_certificate_name', - # Validation service configuration - 'validation_configmap_name', - 'validation_service_name', - 'validation_deployment_name', - 'validation_container_name', - 'validation_extra_volume_name', - 'validation_extra_volume_mountpath', - 'validation_tls_secret_name', - 'validation_issuer_name', - 'validation_certificate_name', - 'validation_state', - # UI Plugin configuration - 'ui_plugin_console_name', - 'ui_plugin_display_name', - 'ui_plugin_service_name', - 'ui_plugin_deployment_name', - 'ui_plugin_container_name', - 'ui_plugin_state', - # API service configuration - 'api_service_name', - 'api_deployment_name', - 'api_container_name', - 'api_tls_secret_name', - 'api_issuer_name', - 'api_certificate_name', - # CLI Download service configuration (calculated/derived values) - 'cli_download_container_name', - 'cli_download_deployment_name', - 'cli_download_route_name', - 'cli_download_service_name', - 'cli_download_state', - # Populator configuration - 'populator_controller_deployment_name', - 'populator_controller_container_name', - # VDDK configuration - 'vddk_build_config_name', - 'vddk_image_stream_name', - # Metrics configuration - 'metric_service_name', - 'metric_servicemonitor_name', - 'metric_interval', - 'metric_port_name', - 'metrics_rule_name', - # Additional internal variables that should not be CRD properties - 'app_namespace', - 'forklift_resources', - 'ui_plugin_console_name', - 'ui_plugin_service_name', - 'validation_service_name', - } - - return variables - ansible_builtin - excluded_fields + return variables - ansible_builtin - EXCLUDED_CRD_FIELDSAdd near the top (module scope):
# Calculated/derived/internal variables that must not be CRD properties EXCLUDED_CRD_FIELDS: FrozenSet[str] = frozenset({ 'app_name', 'app_namespace', 'forklift_operator_version', 'forklift_resources', # Service and component names 'controller_configmap_name', 'controller_service_name', 'controller_deployment_name', 'controller_container_name', 'ovirt_osmap_configmap_name', 'vsphere_osmap_configmap_name', 'virt_customize_configmap_name', 'profiler_volume_path', # Inventory service fields 'inventory_volume_path', 'inventory_container_name', 'inventory_service_name', 'inventory_route_name', 'inventory_route_timeout', 'inventory_tls_secret_name', 'inventory_issuer_name', 'inventory_certificate_name', # Services configuration 'services_service_name', 'services_route_name', 'services_tls_secret_name', 'services_issuer_name', 'services_certificate_name', # Validation service configuration 'validation_configmap_name', 'validation_service_name', 'validation_deployment_name', 'validation_container_name', 'validation_extra_volume_name', 'validation_extra_volume_mountpath', 'validation_tls_secret_name', 'validation_issuer_name', 'validation_certificate_name', # UI Plugin configuration 'ui_plugin_console_name', 'ui_plugin_display_name', 'ui_plugin_service_name', 'ui_plugin_deployment_name', 'ui_plugin_container_name', 'ui_plugin_state', # API service configuration 'api_service_name', 'api_deployment_name', 'api_container_name', 'api_tls_secret_name', 'api_issuer_name', 'api_certificate_name', # CLI Download service configuration (calculated/derived values) 'cli_download_container_name', 'cli_download_deployment_name', 'cli_download_route_name', 'cli_download_service_name', 'cli_download_state', # Populator configuration 'populator_controller_deployment_name', 'populator_controller_container_name', # VDDK configuration 'vddk_build_config_name', 'vddk_image_stream_name', # Metrics configuration 'metric_service_name', 'metric_servicemonitor_name', 'metric_interval', 'metric_port_name', 'metrics_rule_name', })operator/.kustomized_manifests (1)
6466-6467: CLI_DOWNLOAD_IMAGE env wired into Deployment — LGTM.Matches repo automation for image vars/relatedImages noted earlier.
operator/export-vars-downstream.sh (1)
25-26: Ack: placeholder digest — rely on pipeline to replaceZero SHA digest is fine given your automation; just ensure no zero-digest refs remain in generated artifacts before publishing.
Run to confirm no placeholders leak:
#!/bin/bash rg -nP 'sha256:0{64}' || trueoperator/export-vars-upstream.sh (1)
25-26: Upstream image entry added — verify manifest substitutionEntry looks good; confirm your generation step resolves
${CLI_DOWNLOAD_IMAGE}and surfaces it in relatedImages.Quick check:
#!/bin/bash rg -n 'CLI_DOWNLOAD_IMAGE|forklift-cli-download' operator/.upstream_manifests operator/.kustomized_manifests || trueoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 (1)
21-29: Add readiness/liveness probes for nginxImproves rollout safety and self-heal without changing behavior.
containers: - name: "{{ cli_download_container_name }}" image: "{{ cli_download_image_fqin }}" ports: - containerPort: 8080 protocol: TCP imagePullPolicy: "{{ image_pull_policy }}" + readinessProbe: + httpGet: + path: "{{ cli_download_health_path | default('/') }}" + port: 8080 + initialDelaySeconds: 3 + periodSeconds: 10 + failureThreshold: 3 + livenessProbe: + httpGet: + path: "{{ cli_download_health_path | default('/') }}" + port: 8080 + initialDelaySeconds: 10 + periodSeconds: 20 + failureThreshold: 3build/forklift-cli-download/download-latest-release.sh (2)
1-1: Enable strict bash modeFail fast and catch errors early.
-#!/bin/bash +#!/bin/bash +set -Eeuo pipefail
13-16: Fix misleading error messageMessage says “using fallback” but exits. Make it accurate.
-if [ -z "$TAG" ]; then - echo "Could not determine latest tag, using fallback..." - exit 1 -fi +if [ -z "$TAG" ]; then + echo "ERROR: Could not determine latest tag." >&2 + exit 1 +fioperator/.upstream_manifests (2)
6817-6840: Add CLI_DOWNLOAD_IMAGE to CSV relatedImages for disconnected mirroringWithout it, OLM can’t mirror the new image. Apply via generator.
relatedImages: - image: ${OPERATOR_IMAGE} name: forklift-operator @@ - image: ${VSPHERE_XCOPY_VOLUME_POPULATOR_IMAGE} name: vsphere_xcopy_volume_populator + - image: ${CLI_DOWNLOAD_IMAGE} + name: cli_download
6311-6317: RBAC: scope verbs for consoleclidownloads (avoid '*')Limit to required verbs and split from consoleplugins. Apply via generator.
- - consoleplugins - - consoleclidownloads - verbs: - - '*' + - consoleplugins + verbs: + - '*' +- apiGroups: + - console.openshift.io + resources: + - consoleclidownloads + verbs: + - get + - list + - watch + - create + - update + - patch + - deleteoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (1)
83-83: Avoid :latest in examples — use versioned tags (or digests).Replace example FQINs using :latest with pinned tags like quay.io/kubev2v/
:vX.Y.Z to promote reproducibility.
- example: "quay.io/kubev2v/forklift-controller:latest" + example: "quay.io/kubev2v/forklift-controller:vX.Y.Z" - example: "quay.io/kubev2v/forklift-virt-v2v:latest" + example: "quay.io/kubev2v/forklift-virt-v2v:vX.Y.Z" - example: "quay.io/kubev2v/forklift-api:latest" + example: "quay.io/kubev2v/forklift-api:vX.Y.Z" - example: "quay.io/kubev2v/forklift-cli-download:latest" + example: "quay.io/kubev2v/forklift-cli-download:vX.Y.Z" - example: "quay.io/kubev2v/forklift-ui-plugin:latest" + example: "quay.io/kubev2v/forklift-ui-plugin:vX.Y.Z" - example: "quay.io/kubev2v/forklift-validation:latest" + example: "quay.io/kubev2v/forklift-validation:vX.Y.Z" - example: "quay.io/kubev2v/populator-controller:latest" + example: "quay.io/kubev2v/populator-controller:vX.Y.Z" - example: "quay.io/kubev2v/ovirt-populator:latest" + example: "quay.io/kubev2v/ovirt-populator:vX.Y.Z" - example: "quay.io/kubev2v/openstack-populator:latest" + example: "quay.io/kubev2v/openstack-populator:vX.Y.Z" - example: "quay.io/kubev2v/vsphere-populator:latest" + example: "quay.io/kubev2v/vsphere-populator:vX.Y.Z" - example: "quay.io/kubev2v/ova-provider-server:latest" + example: "quay.io/kubev2v/ova-provider-server:vX.Y.Z" - example: "quay.io/kubev2v/forklift-must-gather:latest" + example: "quay.io/kubev2v/forklift-must-gather:vX.Y.Z"Also applies to: 87-87, 95-95, 99-99, 103-103, 107-107, 111-111, 115-115, 119-119, 123-123, 127-127, 131-131
operator/.downstream_manifests (1)
6466-6467: Deployment: CLI_DOWNLOAD_IMAGE env wired — looks good.CSV relatedImages/RELATED_IMAGE_* are handled by automation in this repo; no manual addition needed.
Makefile (2)
308-309: Confirm bundle consumption of CLI_DOWNLOAD_IMAGE (or document automation).If the bundle Containerfiles don’t actually use this ARG and your pipeline substitutes via envsubst/images.conf, that’s fine—just confirm. Otherwise it’s dead ARG.
Run:
#!/bin/bash # Show where the ARG is declared vs. used inside bundle build context fd -a 'Containerfile*' build/forklift-operator-bundle | while read f; do echo "== $f" rg -n 'ARG\s+CLI_DOWNLOAD_IMAGE' "$f" || true rg -n '\bCLI_DOWNLOAD_IMAGE\b' "$f" || true done
361-367: Mark new targets as phony (follow-up ok).Add .PHONY for consistency with future cleanup PR.
+.PHONY: build-cli-download-image push-cli-download-image build-cli-download-image: check_container_runtimebuild/forklift-operator-bundle/Containerfile-downstream (1)
32-33: All-zero digest placeholder — ensure pipeline replacement or drop default.If automation replaces this, OK; otherwise remove the default or surface as ENV to avoid silent drift.
Please confirm your release job replaces sha256:000…000 here and in export scripts.
operator/roles/forkliftcontroller/tasks/main.yml (3)
293-301: Verify cleanup removes ConsoleCLIDownloads when feature disabled.Ensure forklift_resources includes ConsoleCLIDownload or add explicit removal.
#!/bin/bash # Show forklift_resources and confirm ConsoleCLIDownload included rg -n '^forklift_resources:' -C3 operator/roles/forkliftcontroller/defaults --hidden || true # Grep labels in ConsoleCLIDownload templates to match cleanup selector rg -n 'service:\s*\{\{\s*cli_download_service_name\s*\}\}' operator/roles/forkliftcontroller/templates/cli-download -C2 || true
249-252: Use merge patch for Deployment apply (consistency, fewer flips).Add merge_type: "merge" like the controller Deployment.
- name: "Setup CLI download deployment" k8s: state: "{{ cli_download_state }}" definition: "{{ lookup('template', 'cli-download/deployment-cli-download.yml.j2') }}" + merge_type: "merge"
259-268: Gate ConsoleCLIDownload on API presence to avoid rare drifts.Run only if 'console.openshift.io' exists.
- name: "Setup ConsoleCLIDownload for kubectl-mtv" k8s: state: "{{ cli_download_state }}" definition: "{{ lookup('template', 'cli-download/console-cli-download-kubectl-mtv.yml.j2') }}" + when: "'console.openshift.io' in api_groups" - name: "Setup ConsoleCLIDownload for kubectl-mtv MCP" k8s: state: "{{ cli_download_state }}" definition: "{{ lookup('template', 'cli-download/console-cli-download-mcp.yml.j2') }}" + when: "'console.openshift.io' in api_groups"operator/config/rbac/role.yaml (2)
42-47: Tighten verbs for console resources (avoid '*').Limit to get,list,watch,create,update,patch,delete.
resources: - consoleplugins - consoleclidownloads - verbs: - - '*' + verbs: + - get + - list + - watch + - create + - update + - patch + - delete
49-53: Also restrict verbs on routes.Same principle—avoid wildcard.
- resources: - - routes - verbs: - - '*' + resources: + - routes + verbs: + - get + - list + - watch + - create + - update + - patch + - delete
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (6)
build/forklift-cli-download/README.mdis excluded by!**/*.mdbuild/forklift-cli-download/artifacts/kubectl-mtv-darwin-amd64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-darwin-arm64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-linux-amd64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-linux-arm64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-windows-amd64.zipis excluded by!**/*.zip,!**/*.zip
📒 Files selected for processing (22)
Makefile(5 hunks)build/forklift-cli-download/Containerfile(1 hunks)build/forklift-cli-download/Containerfile-downstream(1 hunks)build/forklift-cli-download/download-latest-release.sh(1 hunks)build/forklift-operator-bundle/Containerfile(1 hunks)build/forklift-operator-bundle/Containerfile-downstream(1 hunks)hack/validate_forklift_controller_crd.py(2 hunks)operator/.downstream_manifests(10 hunks)operator/.kustomized_manifests(10 hunks)operator/.upstream_manifests(10 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(2 hunks)operator/config/manager/manager.yaml(1 hunks)operator/config/rbac/role.yaml(1 hunks)operator/export-vars-downstream.sh(2 hunks)operator/export-vars-upstream.sh(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(3 hunks)operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/route-cli-download.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/service-cli-download.yml.j2(1 hunks)
🔥 Files not summarized due to errors (1)
- Makefile: Error: Server error: no LLM provider could handle the message
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.110Z
Learning: Forklift repo: the forklift-cli-download image is part of the standard pipeline — Makefile defines build-cli-download-image/push-cli-download-image and includes them in build-all-images/push-all-images; export-vars {upstream,downstream} also define CLI_DOWNLOAD_IMAGE with downstream replacement to mtv-cli-download-rhel9.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.109Z
Learning: Forklift repo Makefile contains build-cli-download-image and push-cli-download-image targets and includes them in build-all-images/push-all-images; the CLI download image is part of the standard image pipeline.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/export-vars-upstream.sh:25-26
Timestamp: 2025-09-18T13:44:23.321Z
Learning: The yaacov/forklift project has automation that handles the substitution of CLI_DOWNLOAD_IMAGE placeholders (${CLI_DOWNLOAD_IMAGE}) in manifests during the build/release process, similar to how they handle placeholder sha256 digests.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.012Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
📚 Learning: 2025-09-18T14:21:10.110Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.110Z
Learning: Forklift repo: the forklift-cli-download image is part of the standard pipeline — Makefile defines build-cli-download-image/push-cli-download-image and includes them in build-all-images/push-all-images; export-vars {upstream,downstream} also define CLI_DOWNLOAD_IMAGE with downstream replacement to mtv-cli-download-rhel9.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreambuild/forklift-cli-download/Containerfile-downstreamoperator/export-vars-downstream.shbuild/forklift-cli-download/Containerfileoperator/config/manager/manager.yamloperator/roles/forkliftcontroller/defaults/main.ymlbuild/forklift-cli-download/download-latest-release.shoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/roles/forkliftcontroller/tasks/main.ymloperator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/export-vars-upstream.shbuild/forklift-operator-bundle/Containerfileoperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-18T13:44:23.321Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/export-vars-upstream.sh:25-26
Timestamp: 2025-09-18T13:44:23.321Z
Learning: The yaacov/forklift project has automation that handles the substitution of CLI_DOWNLOAD_IMAGE placeholders (${CLI_DOWNLOAD_IMAGE}) in manifests during the build/release process, similar to how they handle placeholder sha256 digests.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreamoperator/config/rbac/role.yamloperator/export-vars-downstream.shbuild/forklift-cli-download/Containerfileoperator/config/manager/manager.yamloperator/roles/forkliftcontroller/defaults/main.ymlbuild/forklift-cli-download/download-latest-release.shoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/roles/forkliftcontroller/tasks/main.ymloperator/.kustomized_manifestsoperator/export-vars-upstream.shbuild/forklift-operator-bundle/Containerfileoperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-18T14:21:10.109Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.109Z
Learning: Forklift repo Makefile contains build-cli-download-image and push-cli-download-image targets and includes them in build-all-images/push-all-images; the CLI download image is part of the standard image pipeline.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreambuild/forklift-cli-download/Containerfile-downstreamoperator/export-vars-downstream.shoperator/config/manager/manager.yamloperator/.kustomized_manifestsoperator/export-vars-upstream.shbuild/forklift-operator-bundle/Containerfile
📚 Learning: 2025-08-19T10:02:47.617Z
Learnt from: yaacov
PR: kubev2v/forklift#2561
File: Makefile:0-0
Timestamp: 2025-08-19T10:02:47.617Z
Learning: User yaacov prefers to defer security hardening improvements like checksum verification to follow-up PRs rather than expanding the scope of cleanup-focused PRs.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreamoperator/config/rbac/role.yamloperator/export-vars-downstream.shoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/roles/forkliftcontroller/tasks/main.ymloperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-18T13:45:03.983Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/rbac/role.yaml:42-47
Timestamp: 2025-09-18T13:45:03.983Z
Learning: In the yaacov/forklift project, comprehensive fixes (like RBAC permission cleanup) are preferred to be done in dedicated PRs rather than mixed with feature additions, to ensure consistency across the codebase.
Applied to files:
Makefileoperator/config/rbac/role.yamlbuild/forklift-cli-download/Containerfile-downstreamoperator/roles/forkliftcontroller/tasks/main.yml
📚 Learning: 2025-08-18T13:45:08.364Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:694-699
Timestamp: 2025-08-18T13:45:08.364Z
Learning: The user yaacov prefers not to change existing behavior within the scope of a current PR when additional validation or safety checks are identified, and instead prefers to create follow-up issues to address such improvements separately.
Applied to files:
Makefileoperator/config/rbac/role.yamlbuild/forklift-cli-download/Containerfile-downstreamoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/roles/forkliftcontroller/tasks/main.ymloperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-18T14:14:19.484Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/manager/manager.yaml:49-50
Timestamp: 2025-09-18T14:14:19.484Z
Learning: In the yaacov/forklift project, RELATED_IMAGE_* environment variables and CSV relatedImages entries are generated automatically by the build automation. Manual addition of RELATED_IMAGE_* env vars in operator/config/manager/manager.yaml is not needed when the base image env var (like CLI_DOWNLOAD_IMAGE) is already present.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreamoperator/export-vars-downstream.shoperator/config/manager/manager.yamloperator/roles/forkliftcontroller/defaults/main.ymloperator/.kustomized_manifestsoperator/export-vars-upstream.shbuild/forklift-operator-bundle/Containerfileoperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-18T13:04:20.561Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-operator-bundle/Containerfile-downstream:32-33
Timestamp: 2025-09-18T13:04:20.561Z
Learning: The yaacov/forklift project uses placeholder sha256 digests (all zeros) in Containerfiles and export scripts, which are automatically replaced by their build automation during the release process.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreamoperator/export-vars-downstream.shbuild/forklift-cli-download/Containerfileoperator/export-vars-upstream.sh
📚 Learning: 2025-09-18T12:49:31.913Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-cli-download/Containerfile:1-4
Timestamp: 2025-09-18T12:49:31.913Z
Learning: The yaacov/forklift project has automation that requires the UBI nginx base image to use the tag format `registry.access.redhat.com/ubi9/nginx-126:1-1756959223` rather than digest pinning in their Containerfiles.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreambuild/forklift-cli-download/Containerfile-downstreamoperator/export-vars-downstream.shbuild/forklift-cli-download/Containerfileoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/export-vars-upstream.sh
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/config/rbac/role.yamlbuild/forklift-cli-download/Containerfile-downstreamoperator/export-vars-downstream.shoperator/config/manager/manager.yamloperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/roles/forkliftcontroller/tasks/main.ymloperator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/export-vars-upstream.shoperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-18T12:50:21.059Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2:21-36
Timestamp: 2025-09-18T12:50:21.059Z
Learning: In the forklift project, yaacov prefers to rely on cluster-level default security context settings rather than explicitly defining securityContext in individual Kubernetes deployment manifests. This allows for centralized security policy management.
Applied to files:
operator/config/rbac/role.yamloperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2
📚 Learning: 2025-08-13T03:31:49.934Z
Learnt from: yaacov
PR: kubev2v/forklift#2464
File: Makefile:138-147
Timestamp: 2025-08-13T03:31:49.934Z
Learning: In the kubev2v/forklift project, PRs should maintain focused scope and avoid mixing concerns. For example, adding a new environment variable should not be combined with larger refactoring efforts in the same PR, even if both changes affect the same file.
Applied to files:
operator/config/rbac/role.yamloperator/.kustomized_manifests
📚 Learning: 2025-09-01T16:34:51.909Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:233-236
Timestamp: 2025-09-01T16:34:51.909Z
Learning: The maintainer yaacov prefers to keep PRs focused on their intended scope and handle different types of changes (like typo fixes vs documentation updates) in separate issues/PRs. When documenting existing APIs, the documentation should accurately reflect the current implementation state, even if it contains typos that will be fixed later.
Applied to files:
operator/config/rbac/role.yamloperator/roles/forkliftcontroller/tasks/main.yml
📚 Learning: 2025-09-18T12:48:54.433Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-cli-download/Containerfile-downstream:10-23
Timestamp: 2025-09-18T12:48:54.433Z
Learning: The forklift project uses "Apache License 2.0" as their standard license string format across all builds, rather than the SPDX identifier "Apache-2.0", for consistency reasons.
Applied to files:
build/forklift-cli-download/Containerfile-downstream
📚 Learning: 2025-09-18T12:50:33.640Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2:24-24
Timestamp: 2025-09-18T12:50:33.640Z
Learning: The forklift project has automation that requires tag-based image references rather than digest-pinned references in their deployment templates.
Applied to files:
build/forklift-cli-download/Containerfileoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-18T13:48:36.012Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.012Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Applied to files:
operator/config/manager/manager.yamloperator/roles/forkliftcontroller/defaults/main.ymloperator/roles/forkliftcontroller/tasks/main.ymloperator/.kustomized_manifestsoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/export-vars-upstream.shoperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-17T03:50:40.352Z
Learnt from: mansam
PR: kubev2v/forklift#2855
File: operator/roles/forkliftcontroller/defaults/main.yml:144-144
Timestamp: 2025-09-17T03:50:40.352Z
Learning: In operator/roles/forkliftcontroller/defaults/main.yml, hooks_container_requests_memory (for hooks containers) and ova_container_requests_memory (for OVA provider server containers) are separate components with different memory requirements and should not be changed together.
Applied to files:
operator/roles/forkliftcontroller/defaults/main.yml
📚 Learning: 2025-09-18T12:47:53.805Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-cli-download/download-latest-release.sh:5-18
Timestamp: 2025-09-18T12:47:53.805Z
Learning: In shell scripts for this project, prefer wget over curl as wget is installed by default while curl may not be available.
Applied to files:
build/forklift-cli-download/download-latest-release.sh
📚 Learning: 2025-08-26T11:37:16.255Z
Learnt from: yaacov
PR: kubev2v/forklift#2633
File: operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2:60-63
Timestamp: 2025-08-26T11:37:16.255Z
Learning: In the forklift project's Jinja2 templates (like operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2), template variables for environment values are typically not quoted when they represent single words or predictable values, maintaining consistency with the existing codebase style.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2
📚 Learning: 2025-09-17T07:04:22.398Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: pkg/controller/plan/util/utils.go:127-138
Timestamp: 2025-09-17T07:04:22.398Z
Learning: The maintainer yaacov strongly prefers to avoid mixing different types of improvements (like security enhancements) with feature implementations in the same PR, even for small optional changes like replacing math/rand with crypto/rand.
Applied to files:
operator/roles/forkliftcontroller/tasks/main.yml
📚 Learning: 2025-08-18T13:44:07.771Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:909-916
Timestamp: 2025-08-18T13:44:07.771Z
Learning: When yaacov requests to defer an improvement to a follow-up issue, create a GitHub issue using the gh CLI with proper title, description, and assignment to yaacov.
Applied to files:
operator/roles/forkliftcontroller/tasks/main.ymloperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-16T12:04:39.609Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:1253-1270
Timestamp: 2025-09-16T12:04:39.609Z
Learning: The files operator/.kustomized_manifests and operator/.upstream_manifests are also auto-generated manifests in the Forklift project, similar to operator/.downstream_manifests.
Applied to files:
operator/.kustomized_manifestsoperator/export-vars-upstream.shoperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-16T12:06:30.507Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/config/crd/bases/forklift.konveyor.io_migrations.yaml:547-565
Timestamp: 2025-09-16T12:06:30.507Z
Learning: The file operator/config/crd/bases/forklift.konveyor.io_migrations.yaml is auto-generated and cannot be directly modified. Any formatting fixes need to be applied to the source templates or generation process that creates this file.
Applied to files:
operator/.kustomized_manifestsoperator/.upstream_manifests
📚 Learning: 2025-09-03T04:08:05.745Z
Learnt from: yaacov
PR: kubev2v/forklift#2694
File: operator/.downstream_manifests:2523-2534
Timestamp: 2025-09-03T04:08:05.745Z
Learning: For Forklift Plan CRD docs, maintainer prefers documenting existing runtime keys/behavior verbatim (e.g., TargetLabels uses "app", convertor pods use "forklift.app") and avoiding normalization changes in documentation-only PRs.
Applied to files:
operator/.kustomized_manifests
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-16T12:04:53.070Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:4344-4361
Timestamp: 2025-09-16T12:04:53.070Z
Learning: The file `operator/.downstream_manifests` in the Forklift project is auto-generated and should not be edited directly. Any formatting issues or changes need to be addressed in the source templates or generation logic that creates this file.
Applied to files:
operator/export-vars-upstream.shoperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-16T12:05:52.685Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.upstream_manifests:1253-1270
Timestamp: 2025-09-16T12:05:52.685Z
Learning: The file operator/.upstream_manifests is auto-generated and cannot be directly modified. Any formatting fixes need to be applied to the source templates or generation process that creates this file.
Applied to files:
operator/.upstream_manifests
📚 Learning: 2025-09-16T12:04:39.609Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:1253-1270
Timestamp: 2025-09-16T12:04:39.609Z
Learning: The file operator/.downstream_manifests is auto-generated and cannot be directly modified. Any formatting fixes need to be applied to the source templates or generation process that creates this file.
Applied to files:
operator/.upstream_manifests
📚 Learning: 2025-09-17T03:54:23.225Z
Learnt from: mansam
PR: kubev2v/forklift#2855
File: operator/.downstream_manifests:6102-6107
Timestamp: 2025-09-17T03:54:23.225Z
Learning: Current OVA server route handling in pkg/controller/provider/ova-setup.go is create-only (no watch/reconcile). RBAC may remain limited to create for route.openshift.io Routes until reconciliation is implemented.
Applied to files:
operator/.downstream_manifests
🪛 Checkov (3.2.334)
operator/roles/forkliftcontroller/tasks/main.yml
[low] 241-270: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Red Hat Konflux / forklift-operator-dev-preview-on-pull-request
- GitHub Check: Red Hat Konflux / forklift-operator-bundle-dev-preview-on-pull-request
- GitHub Check: build
🔇 Additional comments (24)
operator/.kustomized_manifests (2)
63-82: Approve — feature_cli_download defaults are wired and resource defaults present.feature_cli_download is set to true in operator/roles/forkliftcontroller/defaults/main.yml and cli_download_image_fqin plus the container requests/limits are defined there and referenced by the operator's tasks/templates; no changes required.
61-63: Image example FQINs migrated to quay.io/kubev2v — LGTM; scan for stragglers.Change set looks consistent across examples. Quick check for any remaining forklift images under the old org.
Also applies to: 115-118, 268-270, 287-290, 291-294, 295-298, 299-302, 303-306, 323-326, 343-346, 351-354, 384-387
operator/export-vars-downstream.sh (1)
44-44: Downstream replacement key looks correct
replacements[CLI_DOWNLOAD_IMAGE]=mtv-cli-download-rhel9aligns with your image naming. No further action.build/forklift-cli-download/Containerfile-downstream (1)
1-23: LGTM: minimal, consistent downstream imageBase, COPY path, port, and labels align with the rest of the repo.
operator/roles/forkliftcontroller/defaults/main.yml (2)
12-12: Feature gate default ON looks rightMatches PR objective to enable CLI download by default.
102-112: CLI download defaults look saneImage sourcing and resource requests/limits are reasonable for nginx static server.
operator/.upstream_manifests (4)
6466-6467: Approve: CLI_DOWNLOAD_IMAGE present and propagatedMakefile builds/pushes CLI_DOWNLOAD_IMAGE; operator/export-vars-upstream.sh and -downstream.sh set images[CLI_DOWNLOAD_IMAGE] (downstream replacement mtv-cli-download-rhel9); roles/defaults and operator/config/manager/manager.yaml consume it — CSV relatedImages auto-generation should pick this up. No further changes required.
61-63: Verify quay.io/kubev2v images are published and export-vars/templates map correctlyCRD examples and Makefile defaults reference quay.io/kubev2v, while operator/export-vars-upstream.sh and export-vars-downstream.sh are pinned to quay.io/redhat-user-workloads@sha256 and downstream replacements map to mtv-*. Confirm CI publishes the kubev2v images or update export-vars/templates to point to the intended digests/names.
Check: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml, Makefile, operator/export-vars-upstream.sh, operator/export-vars-downstream.sh.
64-82: CRD: CLI downloader resources/image fields are consumedTemplates and defaults use these keys to set the cli-download container image and resource requests/limits.
Locations:
- operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 — image and resources (lines 24, 31–35)
- operator/roles/forkliftcontroller/defaults/main.yml — defaults/env lookup (lines 102, 107–110)
- operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml — CRD entries (lines 96, 188, 192, 196, 200)
179-184: feature_cli_download flag added — gating verifiedService/Deployment/Route and ConsoleCLIDownload are created when feature_cli_download=true (and not k8s_cluster) and removed when false.
- Creation gated in operator/roles/forkliftcontroller/tasks/main.yml: "Set CLI download feature state" (sets cli_download_state when feature_cli_download|bool) and a resource block guarded by when: feature_cli_download|bool and not k8s_cluster|bool (creates service, deployment, route, ConsoleCLIDownload templates).
- Cleanup guarded in operator/roles/forkliftcontroller/tasks/main.yml: when: not feature_cli_download|bool -> include_tasks: cleanup.yml over forklift_resources.
- Templates exist at operator/roles/forkliftcontroller/templates/cli-download/ (service-cli-download.yml.j2, deployment-cli-download.yml.j2, route-cli-download.yml.j2, console-cli-download-*.yml.j2).
- Default and CRD updated: feature_cli_download: true in operator/roles/forkliftcontroller/defaults/main.yml and field present in operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml.
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (4)
91-91: VDDK example already pinned — good.Keeping a versioned tag here is appropriate.
187-204: Verified — resource keys propagate and RBAC present. The deployment template operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 references cli_download_container_limits_cpu, cli_download_container_limits_memory, cli_download_container_requests_cpu and cli_download_container_requests_memory; operator/config/rbac/role.yaml includes consoleclidownloads and route.openshift.io (routes).
96-99: Approve — cli_download_image_fqin addition is correctly wired.
Manager env var, role default, export-vars (upstream & downstream), and Makefile build/push targets for CLI_DOWNLOAD_IMAGE are present and consistent.
74-77: Feature gate addition looks correct — wiring verified; do not expose a single feature_ flag*defaults/main.yml defines feature_cli_download: true and operator/roles/forkliftcontroller/tasks/main.yml honors it (uses feature_cli_download|bool). operator/.upstream_manifests, operator/.kustomized_manifests, and operator/.downstream_manifests currently expose only feature_cli_download — per repo policy, remove this single-flag exposure from the sample CRs/CSV and bundle all feature_* flags in a dedicated follow-up PR.
operator/roles/forkliftcontroller/defaults/main.yml; operator/roles/forkliftcontroller/tasks/main.yml; operator/.upstream_manifests, operator/.kustomized_manifests, operator/.downstream_manifests.operator/.downstream_manifests (4)
6311-6322: RBAC: manager-role now includes consoleclidownloads and routes — resolves Route/CLIDownload creation risks.Scope/verbs are consistent with existing manager-role patterns.
61-63: CRD examples: image org switched to quay.io/kubev2v — consistent across components.No issues.
Also applies to: 117-117, 269-269, 289-289, 293-293, 297-297, 301-301, 305-305, 325-325, 345-345, 353-353, 385-385
179-184: Feature gate: feature_cli_download — verifiedDefault is true in operator/roles/forkliftcontroller/defaults/main.yml; role tasks gate setup/cleanup in operator/roles/forkliftcontroller/tasks/main.yml; CRD field present at operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml.
63-82: CRD: CLI download fields added — wired into defaults, templates, and CRD.
Found references in operator/roles/forkliftcontroller/defaults/main.yml (cli_download_* defaults), operator/roles/forkliftcontroller/templates/cli-download/* (deployment, service, route, ConsoleCLIDownload templates), and operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (CRD schema).Makefile (1)
89-89: LGTM: CLI download image is fully wired into build/push and aggregated targets.Variable, dedicated targets, and inclusion in build-all/push-all look correct.
Also applies to: 361-367, 378-378, 392-392
operator/config/manager/manager.yaml (1)
49-50: LGTM: CLI_DOWNLOAD_IMAGE env is correctly exposed to the operator.Matches the image plumbing elsewhere.
operator/roles/forkliftcontroller/templates/cli-download/route-cli-download.yml.j2 (1)
1-18: LGTM: Route with explicit targetPort and edge TLS.Explicit targetPort avoids ambiguity; config is sane.
operator/roles/forkliftcontroller/templates/cli-download/service-cli-download.yml.j2 (1)
1-21: LGTM: Service exposes 8080 with matching selector.Straightforward and consistent with Route.
operator/roles/forkliftcontroller/tasks/main.yml (1)
241-268: LGTM: CLI download deploy flow gated on OpenShift and feature flag.Blocks and templates look consistent.
build/forklift-cli-download/Containerfile (1)
1-17: LGTM: Minimal nginx image for static CLI artifacts.COPY path, port, and run command align with templates.
|
@mnecas added to the description in the top: Screencast.From.2025-09-18.18-55-33.mp4 |
yaacov
force-pushed
the
cli-download-service
branch
2 times, most recently
from
4483a4f to
2c61b39
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
operator/.upstream_manifests (1)
6817-6840: Add CLI_DOWNLOAD_IMAGE to CSV relatedImages (disconnected mirroring)Without this, OLM cannot mirror the new image on disconnected clusters. Apply via generator.
relatedImages: - image: ${OPERATOR_IMAGE} name: forklift-operator @@ - image: ${VSPHERE_XCOPY_VOLUME_POPULATOR_IMAGE} name: vsphere_xcopy_volume_populator + - image: ${CLI_DOWNLOAD_IMAGE} + name: cli_download
♻️ Duplicate comments (16)
build/forklift-operator-bundle/Containerfile-downstream (1)
32-33: Placeholder digest acknowledged; automation updates it.No change needed here given your pipeline replaces the all-zero sha and injects values from images.conf.
operator/config/rbac/role.yaml (1)
45-47: RBAC additions align with new resources.Allowing consoleclidownloads and routes matches the new templates; tightening verbs can land in the planned follow-up.
Also applies to: 48-53
hack/validate_forklift_controller_crd.py (1)
91-173: Global exclusion set is fine for now; matches feature scope.This keeps CLI download internals out of CRD checks; OK given script’s temporary role.
build/forklift-operator-bundle/Containerfile (1)
15-15: ARG likely unused; remove or promote to ENV before envsubst.envsubst sources export-vars-${STREAM}.sh; ARG won’t flow unless exported.
Apply one of:
-ARG CLI_DOWNLOAD_IMAGE="quay.io/kubev2v/forklift-cli-download:latest" +# ARG not needed; value comes from export-vars-${STREAM}.shor
ARG UI_PLUGIN_IMAGE="quay.io/kubev2v/forklift-console-plugin:latest" -ARG CLI_DOWNLOAD_IMAGE="quay.io/kubev2v/forklift-cli-download:latest" +ARG CLI_DOWNLOAD_IMAGE="quay.io/kubev2v/forklift-cli-download:latest" +ENV CLI_DOWNLOAD_IMAGE="${CLI_DOWNLOAD_IMAGE}"operator/roles/forkliftcontroller/tasks/main.yml (2)
293-301: Verify cleanup also removes ConsoleCLIDownload resourcesEnsure ConsoleCLIDownload is included in forklift_resources and templates carry label service: {{ cli_download_service_name }} so cleanup finds them.
#!/bin/bash # Expect: ConsoleCLIDownload present in forklift_resources and labeled with the service name. rg -nP '^\s*forklift_resources:' -C3 operator/roles/forkliftcontroller/defaults || true rg -n 'kind:\s*ConsoleCLIDownload|service:\s*\{\{\s*cli_download_service_name\s*\}\}' -C2 operator/roles/forkliftcontroller/templates/cli-download || true
241-268: Gate ConsoleCLIDownload by API presence and use merge patch for DeploymentTo match controller behavior and avoid failures on clusters without Console API:
- Add merge_type: "merge" to the CLI deployment task.
- Guard ConsoleCLIDownload tasks with a console.openshift.io API check.
- name: "Setup CLI download deployment" k8s: state: "{{ cli_download_state }}" definition: "{{ lookup('template', 'cli-download/deployment-cli-download.yml.j2') }}" + merge_type: "merge" - name: "Setup ConsoleCLIDownload for kubectl-mtv" k8s: state: "{{ cli_download_state }}" definition: "{{ lookup('template', 'cli-download/console-cli-download-kubectl-mtv.yml.j2') }}" + when: "'console.openshift.io' in api_groups" - name: "Setup ConsoleCLIDownload for kubectl-mtv MCP" k8s: state: "{{ cli_download_state }}" definition: "{{ lookup('template', 'cli-download/console-cli-download-mcp.yml.j2') }}" + when: "'console.openshift.io' in api_groups"Optionally wrap the two ConsoleCLIDownload tasks in a block/rescue to log and continue if creation fails (RBAC/drift).
Makefile (1)
361-367: Mark new targets phony (follow-up OK)Add .PHONY for build/push CLI download targets; can be handled in a sweep PR for consistency with other targets.
+.PHONY: build-cli-download-image push-cli-download-image build-cli-download-image: check_container_runtime $(eval CLI_DOWNLOAD_IMAGE=$(REGISTRY)/$(REGISTRY_ORG)/forklift-cli-download:$(REGISTRY_TAG)) $(CONTAINER_CMD) build -t $(CLI_DOWNLOAD_IMAGE) -f build/forklift-cli-download/Containerfile . push-cli-download-image: build-cli-download-image $(CONTAINER_CMD) push $(CLI_DOWNLOAD_IMAGE)operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 (1)
21-36: Add readiness/liveness probes for nginxProbes improve rollout health and self-healing without changing behavior.
containers: - name: "{{ cli_download_container_name }}" image: "{{ cli_download_image_fqin }}" ports: - containerPort: 8080 protocol: TCP imagePullPolicy: "{{ image_pull_policy }}" + readinessProbe: + httpGet: + path: / + port: 8080 + initialDelaySeconds: 3 + periodSeconds: 10 + livenessProbe: + httpGet: + path: / + port: 8080 + initialDelaySeconds: 10 + periodSeconds: 20 resources: limits: cpu: "{{ cli_download_container_limits_cpu }}" memory: "{{ cli_download_container_limits_memory }}" requests: cpu: "{{ cli_download_container_requests_cpu }}" memory: "{{ cli_download_container_requests_memory }}"build/forklift-cli-download/download-latest-release.sh (1)
1-6: Enable strict mode and make REPO overridable (default to kubev2v).Prevents silent failures and avoids hardcoding a personal repo.
Apply:
#!/bin/bash +set -Eeuo pipefail + -REPO="yaacov/kubectl-mtv" +REPO="${REPO:-kubev2v/kubectl-mtv}" ARTIFACTS_DIR="$(dirname "$0")/artifacts" mkdir -p "${ARTIFACTS_DIR}" # Get the latest release tag from GitHub API echo "Getting latest release tag..." TAG=$(wget -qO- "https://api.github.com/repos/${REPO}/releases/latest" | grep "tag_name" | cut -d '"' -f 4)Also applies to: 9-11
operator/.upstream_manifests (2)
179-184: feature_cli_download flag — LGTM; exposure deferredFlag shape matches other feature_* fields. Per repo preference, surfacing in samples/CSV init can be handled in a follow-up PR covering all flags.
I can open a tracking issue “Expose all feature_* flags in samples and CSV init” and assign it to yaacov if you want.
6311-6317: RBAC: replace wildcards for consoleclidownloads and routes (follow-up OK)Use explicit verbs instead of '*'. Apply via generator; safe to defer as agreed.
- - consoleplugins - - consoleclidownloads - verbs: - - '*' + - consoleplugins + verbs: + - '*' +- apiGroups: + - console.openshift.io + resources: + - consoleclidownloads + verbs: + - get + - list + - watch + - create + - update + - patch + - delete @@ - - routes - verbs: - - '*' + - routes + verbs: + - get + - list + - watch + - create + - update + - patch + - deleteAlso applies to: 6318-6322
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml (1)
83-83: Avoid :latest in examples — use pinned, versioned tags.Follow the existing guidance; prefer concrete versioned tags in examples to discourage floating tags. Keep vddk:7.0.3 as-is.
- example: "quay.io/kubev2v/forklift-controller:latest" + example: "quay.io/kubev2v/forklift-controller:vX.Y.Z" - example: "quay.io/kubev2v/forklift-virt-v2v:latest" + example: "quay.io/kubev2v/forklift-virt-v2v:vX.Y.Z" - example: "quay.io/kubev2v/forklift-api:latest" + example: "quay.io/kubev2v/forklift-api:vX.Y.Z" - example: "quay.io/kubev2v/forklift-cli-download:latest" + example: "quay.io/kubev2v/forklift-cli-download:vX.Y.Z" - example: "quay.io/kubev2v/forklift-ui-plugin:latest" + example: "quay.io/kubev2v/forklift-ui-plugin:vX.Y.Z" - example: "quay.io/kubev2v/forklift-validation:latest" + example: "quay.io/kubev2v/forklift-validation:vX.Y.Z" - example: "quay.io/kubev2v/populator-controller:latest" + example: "quay.io/kubev2v/populator-controller:vX.Y.Z" - example: "quay.io/kubev2v/ovirt-populator:latest" + example: "quay.io/kubev2v/ovirt-populator:vX.Y.Z" - example: "quay.io/kubev2v/openstack-populator:latest" + example: "quay.io/kubev2v/openstack-populator:vX.Y.Z" - example: "quay.io/kubev2v/vsphere-populator:latest" + example: "quay.io/kubev2v/vsphere-populator:vX.Y.Z" - example: "quay.io/kubev2v/ova-provider-server:latest" + example: "quay.io/kubev2v/ova-provider-server:vX.Y.Z" - example: "quay.io/kubev2v/forklift-must-gather:latest" + example: "quay.io/kubev2v/forklift-must-gather:vX.Y.Z"Also applies to: 87-87, 95-95, 96-99, 103-103, 107-107, 111-111, 115-115, 119-119, 123-123, 127-127, 131-131
operator/.downstream_manifests (2)
6314-6317: RBAC: add ConsoleCLIDownload permissions to manager-role — resolves earlier gap.This unblocks creation/management of consoleclidownloads by the operator.
6466-6467: Manager env: CLI_DOWNLOAD_IMAGE wired — aligned with project automation.Base image env var added; RELATED_IMAGE_* and CSV relatedImages are auto-generated from this per repo automation.
operator/.kustomized_manifests (2)
6466-6467: Operator wired CLI_DOWNLOAD_IMAGE env — LGTMMatches the image pipeline/automation that substitutes ${CLI_DOWNLOAD_IMAGE}; no manual RELATED_IMAGE_* needed.
6314-6317: Tighten RBAC for consoleclidownloads and routes (replace wildcard verbs; split rules)Principle of least privilege. Keep consoleplugins as-is if needed, but scope consoleclidownloads and routes to CRUD + get/list/watch.
Apply this diff:
- - apiGroups: - - console.openshift.io - resources: - - consoleplugins - - consoleclidownloads - verbs: - - '*' + - apiGroups: + - console.openshift.io + resources: + - consoleplugins + verbs: + - '*' + - apiGroups: + - console.openshift.io + resources: + - consoleclidownloads + verbs: + - get + - list + - watch + - create + - update + - patch + - delete - - apiGroups: - - route.openshift.io - resources: - - routes - verbs: - - '*' + - apiGroups: + - route.openshift.io + resources: + - routes + verbs: + - get + - list + - watch + - create + - update + - patch + - deleteAlso applies to: 6318-6322
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (7)
build/forklift-cli-download/README.mdis excluded by!**/*.mdbuild/forklift-cli-download/artifacts/kubectl-mtv-darwin-amd64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-darwin-arm64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-linux-amd64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-linux-arm64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-mcp-servers-linux-amd64.tar.gzis excluded by!**/*.gz,!**/*.gzbuild/forklift-cli-download/artifacts/kubectl-mtv-windows-amd64.zipis excluded by!**/*.zip,!**/*.zip
📒 Files selected for processing (22)
Makefile(5 hunks)build/forklift-cli-download/Containerfile(1 hunks)build/forklift-cli-download/Containerfile-downstream(1 hunks)build/forklift-cli-download/download-latest-release.sh(1 hunks)build/forklift-operator-bundle/Containerfile(1 hunks)build/forklift-operator-bundle/Containerfile-downstream(1 hunks)hack/validate_forklift_controller_crd.py(2 hunks)operator/.downstream_manifests(10 hunks)operator/.kustomized_manifests(10 hunks)operator/.upstream_manifests(10 hunks)operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml(2 hunks)operator/config/manager/manager.yaml(1 hunks)operator/config/rbac/role.yaml(1 hunks)operator/export-vars-downstream.sh(2 hunks)operator/export-vars-upstream.sh(1 hunks)operator/roles/forkliftcontroller/defaults/main.yml(2 hunks)operator/roles/forkliftcontroller/tasks/main.yml(3 hunks)operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/route-cli-download.yml.j2(1 hunks)operator/roles/forkliftcontroller/templates/cli-download/service-cli-download.yml.j2(1 hunks)
🧰 Additional context used
🧠 Learnings (30)
📓 Common learnings
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.110Z
Learning: Forklift repo: the forklift-cli-download image is part of the standard pipeline — Makefile defines build-cli-download-image/push-cli-download-image and includes them in build-all-images/push-all-images; export-vars {upstream,downstream} also define CLI_DOWNLOAD_IMAGE with downstream replacement to mtv-cli-download-rhel9.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/export-vars-upstream.sh:25-26
Timestamp: 2025-09-18T13:44:23.321Z
Learning: The yaacov/forklift project has automation that handles the substitution of CLI_DOWNLOAD_IMAGE placeholders (${CLI_DOWNLOAD_IMAGE}) in manifests during the build/release process, similar to how they handle placeholder sha256 digests.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.109Z
Learning: Forklift repo Makefile contains build-cli-download-image and push-cli-download-image targets and includes them in build-all-images/push-all-images; the CLI download image is part of the standard image pipeline.
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.012Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
📚 Learning: 2025-09-18T14:21:10.110Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.110Z
Learning: Forklift repo: the forklift-cli-download image is part of the standard pipeline — Makefile defines build-cli-download-image/push-cli-download-image and includes them in build-all-images/push-all-images; export-vars {upstream,downstream} also define CLI_DOWNLOAD_IMAGE with downstream replacement to mtv-cli-download-rhel9.
Applied to files:
build/forklift-operator-bundle/Containerfileoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2Makefilebuild/forklift-cli-download/Containerfile-downstreambuild/forklift-operator-bundle/Containerfile-downstreamoperator/config/manager/manager.yamlbuild/forklift-cli-download/download-latest-release.shoperator/export-vars-downstream.shoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/roles/forkliftcontroller/tasks/main.ymloperator/.upstream_manifestsoperator/.downstream_manifestsoperator/.kustomized_manifestsoperator/export-vars-upstream.shbuild/forklift-cli-download/Containerfileoperator/roles/forkliftcontroller/defaults/main.yml
📚 Learning: 2025-09-18T14:21:10.109Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.kustomized_manifests:6454-6455
Timestamp: 2025-09-18T14:21:10.109Z
Learning: Forklift repo Makefile contains build-cli-download-image and push-cli-download-image targets and includes them in build-all-images/push-all-images; the CLI download image is part of the standard image pipeline.
Applied to files:
build/forklift-operator-bundle/ContainerfileMakefilebuild/forklift-cli-download/Containerfile-downstreambuild/forklift-operator-bundle/Containerfile-downstreamoperator/config/manager/manager.yamloperator/export-vars-downstream.shoperator/.kustomized_manifestsoperator/export-vars-upstream.sh
📚 Learning: 2025-09-18T13:44:23.321Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/export-vars-upstream.sh:25-26
Timestamp: 2025-09-18T13:44:23.321Z
Learning: The yaacov/forklift project has automation that handles the substitution of CLI_DOWNLOAD_IMAGE placeholders (${CLI_DOWNLOAD_IMAGE}) in manifests during the build/release process, similar to how they handle placeholder sha256 digests.
Applied to files:
build/forklift-operator-bundle/Containerfileoperator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2Makefileoperator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2build/forklift-operator-bundle/Containerfile-downstreamoperator/config/manager/manager.yamlbuild/forklift-cli-download/download-latest-release.shoperator/export-vars-downstream.shoperator/roles/forkliftcontroller/tasks/main.ymlhack/validate_forklift_controller_crd.pyoperator/.upstream_manifestsoperator/config/rbac/role.yamloperator/.downstream_manifestsoperator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2operator/.kustomized_manifestsoperator/export-vars-upstream.shbuild/forklift-cli-download/Containerfileoperator/roles/forkliftcontroller/defaults/main.yml
📚 Learning: 2025-09-18T14:14:19.484Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/manager/manager.yaml:49-50
Timestamp: 2025-09-18T14:14:19.484Z
Learning: In the yaacov/forklift project, RELATED_IMAGE_* environment variables and CSV relatedImages entries are generated automatically by the build automation. Manual addition of RELATED_IMAGE_* env vars in operator/config/manager/manager.yaml is not needed when the base image env var (like CLI_DOWNLOAD_IMAGE) is already present.
Applied to files:
build/forklift-operator-bundle/ContainerfileMakefilebuild/forklift-operator-bundle/Containerfile-downstreamoperator/config/manager/manager.yamloperator/export-vars-downstream.shoperator/.upstream_manifestsoperator/.downstream_manifestsoperator/.kustomized_manifestsoperator/export-vars-upstream.shoperator/roles/forkliftcontroller/defaults/main.yml
📚 Learning: 2025-08-26T11:37:16.255Z
Learnt from: yaacov
PR: kubev2v/forklift#2633
File: operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2:60-63
Timestamp: 2025-08-26T11:37:16.255Z
Learning: In the forklift project's Jinja2 templates (like operator/roles/forkliftcontroller/templates/controller/deployment-controller.yml.j2), template variables for environment values are typically not quoted when they represent single words or predictable values, maintaining consistency with the existing codebase style.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2
📚 Learning: 2025-09-18T12:50:33.640Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2:24-24
Timestamp: 2025-09-18T12:50:33.640Z
Learning: The forklift project has automation that requires tag-based image references rather than digest-pinned references in their deployment templates.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamlbuild/forklift-cli-download/Containerfile
📚 Learning: 2025-09-18T12:49:31.913Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-cli-download/Containerfile:1-4
Timestamp: 2025-09-18T12:49:31.913Z
Learning: The yaacov/forklift project has automation that requires the UBI nginx base image to use the tag format `registry.access.redhat.com/ubi9/nginx-126:1-1756959223` rather than digest pinning in their Containerfiles.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2Makefilebuild/forklift-cli-download/Containerfile-downstreambuild/forklift-operator-bundle/Containerfile-downstreamoperator/export-vars-downstream.shoperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/export-vars-upstream.shbuild/forklift-cli-download/Containerfile
📚 Learning: 2025-09-18T12:50:21.059Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2:21-36
Timestamp: 2025-09-18T12:50:21.059Z
Learning: In the forklift project, yaacov prefers to rely on cluster-level default security context settings rather than explicitly defining securityContext in individual Kubernetes deployment manifests. This allows for centralized security policy management.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2operator/config/rbac/role.yaml
📚 Learning: 2025-08-19T10:02:47.617Z
Learnt from: yaacov
PR: kubev2v/forklift#2561
File: Makefile:0-0
Timestamp: 2025-08-19T10:02:47.617Z
Learning: User yaacov prefers to defer security hardening improvements like checksum verification to follow-up PRs rather than expanding the scope of cleanup-focused PRs.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2Makefilebuild/forklift-operator-bundle/Containerfile-downstreamoperator/export-vars-downstream.shoperator/roles/forkliftcontroller/tasks/main.ymlhack/validate_forklift_controller_crd.pyoperator/.upstream_manifestsoperator/config/rbac/role.yamloperator/.downstream_manifests
📚 Learning: 2025-08-18T13:45:08.364Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:694-699
Timestamp: 2025-08-18T13:45:08.364Z
Learning: The user yaacov prefers not to change existing behavior within the scope of a current PR when additional validation or safety checks are identified, and instead prefers to create follow-up issues to address such improvements separately.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2Makefilebuild/forklift-cli-download/Containerfile-downstreamoperator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2operator/roles/forkliftcontroller/tasks/main.ymlhack/validate_forklift_controller_crd.pyoperator/.upstream_manifestsoperator/config/rbac/role.yamloperator/.downstream_manifests
📚 Learning: 2025-09-02T17:01:19.057Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:84-88
Timestamp: 2025-09-02T17:01:19.057Z
Learning: The maintainer yaacov prefers to keep field names as-is when documenting existing ForkliftController CRD fields in documentation PRs, rather than making consistency improvements that would require operator code changes. Field name standardization should be handled in separate PRs focused on operator changes.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2build/forklift-cli-download/Containerfile-downstreamoperator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2operator/config/manager/manager.yamloperator/export-vars-downstream.shoperator/roles/forkliftcontroller/tasks/main.ymlhack/validate_forklift_controller_crd.pyoperator/.upstream_manifestsoperator/config/rbac/role.yamloperator/.downstream_manifestsoperator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2operator/.kustomized_manifestsoperator/export-vars-upstream.sh
📚 Learning: 2025-09-18T13:45:03.983Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/config/rbac/role.yaml:42-47
Timestamp: 2025-09-18T13:45:03.983Z
Learning: In the yaacov/forklift project, comprehensive fixes (like RBAC permission cleanup) are preferred to be done in dedicated PRs rather than mixed with feature additions, to ensure consistency across the codebase.
Applied to files:
Makefilebuild/forklift-cli-download/Containerfile-downstreamoperator/roles/forkliftcontroller/tasks/main.ymloperator/config/rbac/role.yaml
📚 Learning: 2025-09-18T13:04:20.561Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-operator-bundle/Containerfile-downstream:32-33
Timestamp: 2025-09-18T13:04:20.561Z
Learning: The yaacov/forklift project uses placeholder sha256 digests (all zeros) in Containerfiles and export scripts, which are automatically replaced by their build automation during the release process.
Applied to files:
Makefilebuild/forklift-operator-bundle/Containerfile-downstreamoperator/export-vars-downstream.shoperator/export-vars-upstream.shbuild/forklift-cli-download/Containerfile
📚 Learning: 2025-09-18T12:48:54.433Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-cli-download/Containerfile-downstream:10-23
Timestamp: 2025-09-18T12:48:54.433Z
Learning: The forklift project uses "Apache License 2.0" as their standard license string format across all builds, rather than the SPDX identifier "Apache-2.0", for consistency reasons.
Applied to files:
build/forklift-cli-download/Containerfile-downstream
📚 Learning: 2025-09-18T16:01:02.701Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2:12-13
Timestamp: 2025-09-18T16:01:02.701Z
Learning: In Forklift templates using Route host lookup, it's acceptable to have "bad route" fallback URLs when Route status is empty during initial deployment - the complexity of safe lookup chains isn't required for CLI download functionality.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2
📚 Learning: 2025-09-18T13:48:36.012Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: operator/.upstream_manifests:179-184
Timestamp: 2025-09-18T13:48:36.012Z
Learning: Repo preference (kubev2v/forklift): Do not surface individual feature_* flags in sample CRs or CSV init piecemeal. When adding a new feature flag (e.g., feature_cli_download in operator/.upstream_manifests), bundle updates to expose all feature_* flags together in a dedicated follow-up PR rather than updating only the new flag.
Applied to files:
operator/config/manager/manager.yamloperator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yamloperator/roles/forkliftcontroller/tasks/main.ymloperator/.upstream_manifestsoperator/.downstream_manifestsoperator/.kustomized_manifestsoperator/export-vars-upstream.shoperator/roles/forkliftcontroller/defaults/main.yml
📚 Learning: 2025-09-18T12:47:53.805Z
Learnt from: yaacov
PR: kubev2v/forklift#2882
File: build/forklift-cli-download/download-latest-release.sh:5-18
Timestamp: 2025-09-18T12:47:53.805Z
Learning: In shell scripts for this project, prefer wget over curl as wget is installed by default while curl may not be available.
Applied to files:
build/forklift-cli-download/download-latest-release.sh
📚 Learning: 2025-09-01T16:33:01.755Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/.downstream_manifests:172-213
Timestamp: 2025-09-01T16:33:01.755Z
Learning: The ForkliftController operator is architecturally designed to accept only string values for all configuration fields, including feature gates that would typically be booleans. This is why the CRD schema uses string types with enum constraints (like "true"/"false") rather than actual boolean types.
Applied to files:
operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml
📚 Learning: 2025-09-01T16:34:51.909Z
Learnt from: yaacov
PR: kubev2v/forklift#2690
File: operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml:233-236
Timestamp: 2025-09-01T16:34:51.909Z
Learning: The maintainer yaacov prefers to keep PRs focused on their intended scope and handle different types of changes (like typo fixes vs documentation updates) in separate issues/PRs. When documenting existing APIs, the documentation should accurately reflect the current implementation state, even if it contains typos that will be fixed later.
Applied to files:
operator/roles/forkliftcontroller/tasks/main.ymloperator/config/rbac/role.yaml
📚 Learning: 2025-09-17T07:04:22.398Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: pkg/controller/plan/util/utils.go:127-138
Timestamp: 2025-09-17T07:04:22.398Z
Learning: The maintainer yaacov strongly prefers to avoid mixing different types of improvements (like security enhancements) with feature implementations in the same PR, even for small optional changes like replacing math/rand with crypto/rand.
Applied to files:
operator/roles/forkliftcontroller/tasks/main.yml
📚 Learning: 2025-08-18T13:44:07.771Z
Learnt from: yaacov
PR: kubev2v/forklift#2540
File: pkg/controller/plan/adapter/vsphere/builder.go:909-916
Timestamp: 2025-08-18T13:44:07.771Z
Learning: When yaacov requests to defer an improvement to a follow-up issue, create a GitHub issue using the gh CLI with proper title, description, and assignment to yaacov.
Applied to files:
operator/roles/forkliftcontroller/tasks/main.ymloperator/.upstream_manifestsoperator/.downstream_manifests
📚 Learning: 2025-09-16T12:06:30.507Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/config/crd/bases/forklift.konveyor.io_migrations.yaml:547-565
Timestamp: 2025-09-16T12:06:30.507Z
Learning: The file operator/config/crd/bases/forklift.konveyor.io_migrations.yaml is auto-generated and cannot be directly modified. Any formatting fixes need to be applied to the source templates or generation process that creates this file.
Applied to files:
hack/validate_forklift_controller_crd.pyoperator/.upstream_manifests
📚 Learning: 2025-09-16T12:04:39.609Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:1253-1270
Timestamp: 2025-09-16T12:04:39.609Z
Learning: The files operator/.kustomized_manifests and operator/.upstream_manifests are also auto-generated manifests in the Forklift project, similar to operator/.downstream_manifests.
Applied to files:
operator/.upstream_manifestsoperator/.downstream_manifestsoperator/.kustomized_manifestsoperator/export-vars-upstream.sh
📚 Learning: 2025-09-16T12:04:53.070Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.downstream_manifests:4344-4361
Timestamp: 2025-09-16T12:04:53.070Z
Learning: The file `operator/.downstream_manifests` in the Forklift project is auto-generated and should not be edited directly. Any formatting issues or changes need to be addressed in the source templates or generation logic that creates this file.
Applied to files:
operator/.upstream_manifestsoperator/export-vars-upstream.sh
📚 Learning: 2025-09-16T12:05:52.685Z
Learnt from: yaacov
PR: kubev2v/forklift#2812
File: operator/.upstream_manifests:1253-1270
Timestamp: 2025-09-16T12:05:52.685Z
Learning: The file operator/.upstream_manifests is auto-generated and cannot be directly modified. Any formatting fixes need to be applied to the source templates or generation process that creates this file.
Applied to files:
operator/.upstream_manifests
📚 Learning: 2025-08-13T03:31:49.934Z
Learnt from: yaacov
PR: kubev2v/forklift#2464
File: Makefile:138-147
Timestamp: 2025-08-13T03:31:49.934Z
Learning: In the kubev2v/forklift project, PRs should maintain focused scope and avoid mixing concerns. For example, adding a new environment variable should not be combined with larger refactoring efforts in the same PR, even if both changes affect the same file.
Applied to files:
operator/config/rbac/role.yaml
📚 Learning: 2025-09-17T03:54:23.225Z
Learnt from: mansam
PR: kubev2v/forklift#2855
File: operator/.downstream_manifests:6102-6107
Timestamp: 2025-09-17T03:54:23.225Z
Learning: Current OVA server route handling in pkg/controller/provider/ova-setup.go is create-only (no watch/reconcile). RBAC may remain limited to create for route.openshift.io Routes until reconciliation is implemented.
Applied to files:
operator/.downstream_manifests
📚 Learning: 2025-08-26T11:36:02.177Z
Learnt from: yaacov
PR: kubev2v/forklift#2633
File: operator/roles/forkliftcontroller/tasks/main.yml:33-45
Timestamp: 2025-08-26T11:36:02.177Z
Learning: In the forklift project, the pattern `when: "'route.openshift.io' in api_groups"` is used consistently throughout the codebase without additional error handling. Maintain this consistency rather than adding error handling to individual instances.
Applied to files:
operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2
📚 Learning: 2025-09-17T03:50:40.352Z
Learnt from: mansam
PR: kubev2v/forklift#2855
File: operator/roles/forkliftcontroller/defaults/main.yml:144-144
Timestamp: 2025-09-17T03:50:40.352Z
Learning: In operator/roles/forkliftcontroller/defaults/main.yml, hooks_container_requests_memory (for hooks containers) and ova_container_requests_memory (for OVA provider server containers) are separate components with different memory requirements and should not be changed together.
Applied to files:
operator/roles/forkliftcontroller/defaults/main.yml
🪛 Checkov (3.2.334)
operator/roles/forkliftcontroller/tasks/main.yml
[low] 241-270: Ensure block is handling task errors properly
(CKV2_ANSIBLE_3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / forklift-operator-dev-preview-on-pull-request
- GitHub Check: Red Hat Konflux / forklift-operator-bundle-dev-preview-on-pull-request
🔇 Additional comments (26)
operator/roles/forkliftcontroller/templates/cli-download/service-cli-download.yml.j2 (1)
11-18: Service looks good and matches deployment selectors.Port naming (8080-tcp) is consistent; pairs with the Route after switching Route to use the name.
operator/config/manager/manager.yaml (1)
49-50: Wiring looks correct; no extra RELATED_IMAGE needed.Defaults set cli_download_image_fqin = lookup('env','CLI_DOWNLOAD_IMAGE') or RELATED_IMAGE_CLI_DOWNLOAD (operator/roles/forkliftcontroller/defaults/main.yml:102) and the cli-download deployment uses that variable for the image (operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2:24). Keeping CLI_DOWNLOAD_IMAGE in manager.yaml is sufficient.
operator/roles/forkliftcontroller/tasks/main.yml (1)
9-12: LGTM — feature flag wiringSetting cli_download_state via feature_cli_download is correct and consistent with other features.
operator/export-vars-downstream.sh (1)
25-26: LGTM — downstream image and replacement mapping addedimages[CLI_DOWNLOAD_IMAGE] and replacements[CLI_DOWNLOAD_IMAGE] look correct and align with the pipeline.
Also applies to: 44-45
build/forklift-cli-download/Containerfile (1)
1-17: LGTM — minimal nginx-based artifact serverImage, copy path, port, and labels look good for the download service.
Makefile (3)
89-90: LGTM — component image variable addedCLI_DOWNLOAD_IMAGE default fits existing pattern.
308-309: LGTM — passing CLI_DOWNLOAD_IMAGE to bundle buildArg propagation is consistent with other images.
378-379: LGTM — included in batch build/push targetsAdding CLI download image to build-all-images/push-all-images is correct.
Also applies to: 392-393
build/forklift-cli-download/Containerfile-downstream (1)
1-24: LGTM — downstream ContainerfileBase image, ARGs, labels, and exposure align with the downstream flow.
operator/export-vars-upstream.sh (1)
25-26: LGTM — upstream image entry addedCLI_DOWNLOAD_IMAGE export follows existing substitution logic.
operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-mcp.yml.j2 (1)
1-14: LGTM — ConsoleCLIDownload (MCP) templateCluster-scoped resource with route-based URL looks good.
operator/roles/forkliftcontroller/templates/cli-download/console-cli-download-kubectl-mtv.yml.j2 (1)
4-5: ConsoleCLIDownload scope fix looks good.No namespace present on a cluster-scoped resource.
operator/roles/forkliftcontroller/defaults/main.yml (2)
102-111: CLI download defaults look consistent with existing patterns.Image sourcing and resource requests/limits are reasonable.
12-12: CLI download tasks gating verified — no action required.operator/roles/forkliftcontroller/tasks/main.yml sets cli_download_state='present' when feature_cli_download|bool (line 11); the Service/Deployment/Route/ConsoleCLIDownload tasks use {{ cli_download_state }} (≈lines 244–266); cleanup runs when feature_cli_download is false (≈line 293). Tasks are additionally gated by not k8s_cluster.
build/forklift-cli-download/download-latest-release.sh (1)
44-44: Verify MCP servers asset naming (tag vs. no-tag).Could not confirm — GitHub API returned no release assets for kubev2v/kubectl-mtv when queried from the sandbox. Confirm whether the real release asset name includes ${TAG} and update build/forklift-cli-download/download-latest-release.sh (line 44) to use the correct filename or to programmatically resolve the asset name from the release assets.
operator/.upstream_manifests (3)
63-82: CLI download CRD fields — LGTMFields and examples align with existing patterns (string types, resource hints). No blockers.
61-63: Image example fqins switched to kubev2v — LGTMConsistent updates across fields; no schema drift.
Also applies to: 117-117, 269-269, 289-305, 325-325, 345-345, 353-353, 386-386
6466-6467: Operator env CLI_DOWNLOAD_IMAGE — wired OK; CSV relatedImages missing
- Env present: operator/.upstream_manifests (lines 6466–6467) and operator/config/manager/manager.yaml (lines 49–50).
- CSV relatedImages (operator/.downstream_manifests ≈ line 6811) lists OPERATOR_IMAGE and CONTROLLER_IMAGE but does not include CLI_DOWNLOAD_IMAGE.
- build/Makefile not found in repo; confirm CI/build injects RELATED_IMAGE_CLI_DOWNLOAD into the CSV relatedImages or add an explicit relatedImages entry for the CLI download.
operator/.downstream_manifests (5)
115-118: Example image updated (controller) — OK.Just an example string; consistent with other image fields.
179-184: New feature flag feature_cli_download — matches repo conventions.Enum "true"/"false" and default noted in description. Also not surfaced in sample CR (preferred).
Please confirm the operator defaults set this to "true" by default in roles/defaults and that toggling it gates creation of the CLI resources.
269-269: Bulk example image updates — consistent across CRD docs.All example FQINs point to quay.io/kubev2v/*:latest like neighboring fields. No action.
Also applies to: 289-289, 293-293, 297-297, 301-301, 305-305, 325-325, 345-345, 353-353, 386-386
61-82: LGTM — CLI download knobs and operator wiring verified.feature_cli_download is set in operator/roles/forkliftcontroller/defaults/main.yml; tasks are gated on feature_cli_download in operator/roles/forkliftcontroller/tasks/main.yml; templates reference cli_download_image_fqin (operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2); Makefile/export-vars expose CLI_DOWNLOAD_IMAGE.
6318-6323: RBAC: narrow cluster-scope Route verbs (recommended — verify before tightening)Works as-is. Optional: replace verbs: ["get","list","create","update","patch"] instead of "*". Ansible creates Routes in operator/roles/forkliftcontroller/tasks/main.yml (templates controller/route-inventory.yml.j2, api/route-services.yml.j2) using k8s state: present; repo learning: pkg/controller/provider/ova-setup.go currently only creates routes (no watch/reconcile). Verify other controllers/watchers before removing watch or further tightening.
operator/.kustomized_manifests (3)
179-184: Feature gate added: feature_cli_download — exposure policy OKEnum matches existing feature_* fields. Not surfaced piecemeal in sample CR/CSV here, which aligns with the repo preference to batch all flags later. No action.
61-63: Image example references switched to quay.io/kubev2v — LGTMExamples now use kubev2v-prefixed images consistently across CRD fields.
Also applies to: 115-118, 269-270, 289-290, 293-294, 297-298, 301-302, 305-306, 325-326, 345-346, 353-354, 385-387
63-82: CRD: CLI download fields wired — defaults & feature gate verified
- operator/roles/forkliftcontroller/defaults/main.yml defines feature_cli_download: true and sets cli_download_image_fqin, cli_download_container_name, cli_download_container_limits_cpu/memory, cli_download_container_requests_cpu/memory.
- operator/roles/forkliftcontroller/templates/cli-download/deployment-cli-download.yml.j2 consumes the image and resource vars.
- operator/roles/forkliftcontroller/tasks/main.yml gates creation with when: feature_cli_download|bool (and variants like when: feature_cli_download|bool and not k8s_cluster|bool).
- operator/config/crd/bases/forklift.konveyor.io_forkliftcontrollers.yaml exposes the CRD fields.
Signed-off-by: yaacov <kobi.zamir@gmail.com>
yaacov
force-pushed
the
cli-download-service
branch
from
73f2782 to
1fc386e
Compare
4 participants
Ref:
https://issues.redhat.com/browse/MTV-2947
Issue:
Add an option to download MTV CLI and MCP tools
Fix:
Screenshots:
Download:
Clip:
https://github.com/user-attachments/assets/b790ccc3-251a-4eb6-b75d-4638ad845364
Summary by CodeRabbit