Add tests infrastructure and automated tests.#182
Merged
msajidmansoori12 merged 34 commits intomigtools:mainfrom Mar 26, 2026
Merged
Add tests infrastructure and automated tests.#182msajidmansoori12 merged 34 commits intomigtools:mainfrom
msajidmansoori12 merged 34 commits intomigtools:mainfrom
Conversation
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces comprehensive end-to-end testing infrastructure for Crane via GitHub Actions workflows, a Bash provisioning script for dual Minikube clusters on a shared network, and a Go-based test framework with Ginkgo test suites for validating stateless and stateful application migrations across clusters. Changes
Sequence Diagram(s)sequenceDiagram
participant Test as E2E Test
participant SrcCluster as Source Cluster
participant Crane as Crane Binary
participant TgtCluster as Target Cluster
participant Validate as Validation
Test->>SrcCluster: Deploy source app
Test->>SrcCluster: Validate source app
Test->>SrcCluster: Scale down (0 replicas)
Test->>Crane: Export resources from src
Crane-->>Test: Export artifacts
Test->>Crane: Transform exported artifacts
Crane-->>Test: Transform artifacts
Test->>Crane: Apply to generate output manifests
Crane-->>Test: Output manifests
Test->>TgtCluster: Create namespace
Test->>TgtCluster: Validate manifests (dry-run)
Test->>TgtCluster: Apply manifests
alt Stateful Migration
Test->>Crane: Transfer PVCs
Crane->>TgtCluster: Sync PVC data
Crane-->>Test: Transfer complete
end
Test->>TgtCluster: Scale up target deployment (1 replica)
Test->>Validate: Validate target app
Validate-->>Test: Validation result
Test->>Test: Cleanup (temp dirs, resources)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
force-pushed
the
test-setup-minikube
branch
from
4decd65 to
2c1d193
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (14)
.github/workflows/run-e2e-tests.yml (1)
51-57: Consider enabling--verbose-logsfor CI debugging.The test suite registers a
--verbose-logsflag (pere2e/tests/e2e_suite_test.go), but it's not passed here. Enabling it could help diagnose CI failures.♻️ Optional: add verbose logs flag
- name: Run crane e2e tests run: | ginkgo run -v e2e/tests -- \ --k8sdeploy-bin="k8sdeploy" \ --crane-bin="${{ github.workspace }}/crane" \ --source-context=src \ - --target-context=tgt + --target-context=tgt \ + --verbose-logs=true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/run-e2e-tests.yml around lines 51 - 57, Add the missing test flag by passing the registered --verbose-logs flag into the ginkgo invocation so CI captures detailed test output; update the ginkgo run command that constructs the e2e invocation (the block invoking "ginkgo run -v e2e/tests --" with flags like --k8sdeploy-bin and --crane-bin) to include --verbose-logs (and set it to true or the desired level) alongside the existing flags.e2e/tests/e2e_suite_test.go (1)
22-29: HardcodedreporterConfig.Verbose = trueoverrides CLI settings.The reporter verbosity is forced to
true, which overrides any--ginkgo.vor--ginkgo.vvsettings from the command line. This may be intentional for E2E visibility, but worth noting if CI logs become too verbose.♻️ Optional: make verbosity conditional
func TestE2E(t *testing.T) { RegisterFailHandler(Fail) suiteConfig, reporterConfig := GinkgoConfiguration() - reporterConfig.Verbose = true + // Keep CLI verbosity or default to verbose for E2E + if !reporterConfig.Verbose { + reporterConfig.Verbose = true + } log.SetOutput(GinkgoWriter) log.SetFlags(log.LstdFlags) RunSpecs(t, "E2E Suite", suiteConfig, reporterConfig) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/e2e_suite_test.go` around lines 22 - 29, In TestE2E, the line reporterConfig.Verbose = true force-enables verbosity and overrides CLI flags; remove that assignment so reporterConfig retains CLI-provided settings (i.e., delete or comment out reporterConfig.Verbose = true in TestE2E) or, if intentional, replace it with a conditional that only sets verbose when an explicit override is present (e.g., check an env var) to avoid clobbering Ginkgo CLI flags.e2e/utils/utils.go (1)
50-57: Consider avoiding fragile string matching for empty-directory detection.
HasFilesRecursivelyrelies on matching the literal"(no files)"substring returned byListFilesRecursively. If that sentinel string ever changes, this check silently breaks.♻️ Proposed refactor to decouple detection from formatting
-// HasFilesRecursively reports whether a directory contains any files. -func HasFilesRecursively(dir string) (bool, string, error) { - files, err := ListFilesRecursively(dir) - if err != nil { - return false, "", err - } - hasFiles := !strings.Contains(files, "(no files)") - return hasFiles, files, nil +// listFilesRecursivelyRaw returns the raw list of files and the formatted string. +func listFilesRecursivelyRaw(dir string) ([]string, string, error) { + var files []string + err := filepath.Walk(dir, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + return nil + } + files = append(files, path) + return nil + }) + if err != nil { + return nil, "", err + } + + sort.Strings(files) + if len(files) == 0 { + return files, " (no files)", nil + } + + var b strings.Builder + for _, path := range files { + rel, err := filepath.Rel(dir, path) + if err != nil { + rel = path + } + b.WriteString(fmt.Sprintf(" - %s\n", rel)) + } + + return files, strings.TrimRight(b.String(), "\n"), nil +} + +// ListFilesRecursively returns a formatted list of files under a directory. +func ListFilesRecursively(dir string) (string, error) { + _, formatted, err := listFilesRecursivelyRaw(dir) + return formatted, err +} + +// HasFilesRecursively reports whether a directory contains any files. +func HasFilesRecursively(dir string) (bool, string, error) { + files, formatted, err := listFilesRecursivelyRaw(dir) + if err != nil { + return false, "", err + } + return len(files) > 0, formatted, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/utils/utils.go` around lines 50 - 57, HasFilesRecursively is brittle because it checks for the literal "(no files)" in the string returned by ListFilesRecursively; instead change ListFilesRecursively to return a structured result (e.g., ([]string, error) or a struct with a Names []string) and update HasFilesRecursively to call that new API and determine emptiness via len(names) > 0, then separately format/join the names for the string return; update references to ListFilesRecursively and the HasFilesRecursively signature/return to use the new types (functions: ListFilesRecursively, HasFilesRecursively) so emptiness detection no longer depends on string formatting.e2e/framework/client.go (1)
84-84: Error message should start with lowercase per Go conventions.Go error strings conventionally start with a lowercase letter since they are often wrapped or concatenated with other messages.
♻️ Proposed fix
- return "", fmt.Errorf("No node IP found") + return "", fmt.Errorf("no node IP found for context %q", contextName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/framework/client.go` at line 84, The error string created with fmt.Errorf currently uses "No node IP found" which violates Go convention to start errors with a lowercase letter; update the fmt.Errorf call (the return "", fmt.Errorf(...) expression in e2e/framework/client.go) to use a lowercase-leading message such as "no node IP found" so the error adheres to Go error-string conventions..github/actions/setup-minikube-clusters/action.yml (2)
60-66: Hardcoded profile names may cause issues if customized.The script (
hack/setup-minikube-same-network.sh) allows customizing profile names viaSRC_PROFILEandTGT_PROFILEenvironment variables, but this action hardcodessrcandtgtin the verify step. If users need different profile names, this verification will fail.♻️ Suggested fix to use consistent profile names
- name: Verify profiles shell: bash + env: + SRC_PROFILE: src + TGT_PROFILE: tgt run: | minikube profile list - minikube ip -p src - minikube ip -p tgt + minikube ip -p "$SRC_PROFILE" + minikube ip -p "$TGT_PROFILE"Or add
src_profileandtgt_profileas action inputs for full customization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-minikube-clusters/action.yml around lines 60 - 66, The verification step currently hardcodes minikube profile names ("minikube ip -p src" and "minikube ip -p tgt"), which will break when users set SRC_PROFILE/TGT_PROFILE or choose different names; update the action to reference configurable profile names instead (use the environment variables SRC_PROFILE and TGT_PROFILE from hack/setup-minikube-same-network.sh or add action inputs like src_profile and tgt_profile) and replace the hardcoded calls in the verify step (the "Verify profiles" shell run block) to use those variables so the verification matches the configured profiles.
67-77: Redundant context switch beforekubectl run --context=src.Line 71 runs
kubectl config use-context src, then line 72 uses--context=srcexplicitly. The explicit--contextflag makes the prior context switch unnecessary.♻️ Remove redundant context switch
- name: Verify src pod can reach tgt API shell: bash run: | TGT_IP="$(minikube ip -p tgt)" - kubectl config use-context src kubectl run test-connectivity \ --context=src \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/actions/setup-minikube-clusters/action.yml around lines 67 - 77, The step redundantly switches context then passes --context=src; remove the unnecessary kubectl config use-context src line and rely solely on the explicit --context=src in the kubectl run invocation (retain the TGT_IP assignment and the kubectl run test-connectivity ... --context=src ... curl ... command) so the action does not perform an extra global context change.e2e/framework/app.go (1)
82-96: DuplicatePATHentry appended instead of replaced.
os.Environ()returns the current environment includingPATH. Line 95 appends a newPATH=...entry, resulting in duplicatePATHvariables. While most Unix systems use the last occurrence, this is non-idiomatic and may cause confusion or issues on some platforms.♻️ Proposed fix to replace PATH instead of duplicating
func envWithBinDir(bin string) []string { env := os.Environ() if !strings.Contains(bin, string(filepath.Separator)) { return env } binDir := filepath.Dir(bin) pathVal := os.Getenv("PATH") updatedPath := binDir if pathVal != "" { updatedPath = binDir + string(os.PathListSeparator) + pathVal } - return append(env, "PATH="+updatedPath) + // Replace existing PATH entry instead of appending duplicate + result := make([]string, 0, len(env)) + for _, e := range env { + if !strings.HasPrefix(e, "PATH=") { + result = append(result, e) + } + } + return append(result, "PATH="+updatedPath) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/framework/app.go` around lines 82 - 96, The envWithBinDir function currently appends a new "PATH=..." entry to the env slice, causing duplicate PATH entries; modify envWithBinDir to compute updatedPath (using filepath.Dir(bin) and os.PathListSeparator) and then search the env slice returned by os.Environ() for an existing entry with prefix "PATH=" and replace that element with "PATH="+updatedPath; if no existing PATH entry is found, append the new "PATH=" entry. Ensure you still return env and keep the early return when bin has no path separator.hack/setup-minikube-same-network.sh (2)
171-179: Fragile hardcoded port index for hostPort patch.The JSON patch targets
/spec/template/spec/containers/0/ports/1/hostPort(or/0/as fallback), assuming port 443 is at index 0 or 1. If the ingress-nginx container's port ordering changes, this patch will fail or modify the wrong port.Consider using a strategic merge patch or computing the correct index dynamically based on
containerPort==443.♻️ Alternative: use kubectl set command or compute index
# Find the index of port 443 dynamically port_index=$(kubectl get deployment ingress-nginx-controller -n ingress-nginx --context="$TGT_PROFILE" \ -o jsonpath='{range .spec.template.spec.containers[0].ports[*]}{.containerPort}{"\n"}{end}' | \ grep -n "^443$" | cut -d: -f1) if [[ -n "$port_index" ]]; then port_index=$((port_index - 1)) # Convert to 0-based index kubectl patch deployment ingress-nginx-controller -n ingress-nginx --context="$TGT_PROFILE" --type='json' \ -p="[{\"op\":\"add\",\"path\":\"/spec/template/spec/containers/0/ports/${port_index}/hostPort\",\"value\":443}]" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/setup-minikube-same-network.sh` around lines 171 - 179, The patch is using a fragile hardcoded port index when adding hostPort (see https_hostport variable and the kubectl patch calls that target /spec/template/spec/containers/0/ports/1/hostPort and /0/), which can break if port ordering changes; update the script to compute the correct container port index dynamically (query .spec.template.spec.containers[0].ports[*] for containerPort==443, convert to 0-based index) and then use that index in the JSON patch path, or replace the JSON patch with a strategic merge/set command that matches by containerPort instead of an array index.
37-37: Duplicateset -xstatement.
set -xis already enabled on line 3. This duplicate on line 37 is harmless but unnecessary.♻️ Remove duplicate
RECREATE_NETWORK="${RECREATE_NETWORK:-true}" -set -x🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/setup-minikube-same-network.sh` at line 37, Remove the duplicate shell debugging directive by deleting the redundant "set -x" instance (the second occurrence) so only the initial "set -x" remains; locate the repeated "set -x" token in the script and remove that line to avoid the unnecessary duplicate.e2e/tests/stateless_migration_test.go (1)
59-122: Consider removing or tracking the commented-out test.This large commented-out test block (
[MTC-129] No PVCs stage warning) adds noise to the file. If it's work-in-progress, consider tracking it in an issue and removing the commented code. If it's intentionally disabled, add a brief comment explaining why.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/stateless_migration_test.go` around lines 59 - 122, Remove or explicitly document the large commented-out Ginkgo test block starting with the It("[MTC-129] No PVCs stage warning") declaration: either delete the entire commented block to reduce noise, or replace the comment with a short explanatory comment that it’s intentionally disabled and reference a tracking issue (e.g. MTC-129) or move the test content into an issue/PR draft. Locate the block by searching for the test title "[MTC-129] No PVCs stage warning" (which contains symbols like NewMigrationScenario, TransferPVC, RunCranePipelineWithChecks, ApplyOutputToTarget) and apply the chosen cleanup so the file no longer contains that large commented-out test.e2e/framework/scenario.go (3)
60-65: Usefilepath.Joinfor cross-platform path construction.String concatenation with
/may cause issues on Windows. Usepath/filepathfor portable path handling.♻️ Suggested fix
import ( "log" "os" + "path/filepath" "github.com/konveyor/crane/e2e/utils" )return ScenarioPaths{ TempDir: tempDir, - ExportDir: tempDir + "/export", - TransformDir: tempDir + "/transform", - OutputDir: tempDir + "/output", + ExportDir: filepath.Join(tempDir, "export"), + TransformDir: filepath.Join(tempDir, "transform"), + OutputDir: filepath.Join(tempDir, "output"), }, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/framework/scenario.go` around lines 60 - 65, The code returns ScenarioPaths with directories built via string concatenation (TempDir + "/export", "/transform", "/output"); change these to use filepath.Join from the path/filepath package to ensure cross-platform correctness—import "path/filepath" if missing and replace ExportDir: tempDir + "/export", TransformDir: tempDir + "/transform", OutputDir: tempDir + "/output" with filepath.Join(tempDir, "export"), filepath.Join(tempDir, "transform"), filepath.Join(tempDir, "output") respectively while keeping TempDir as-is.
37-38: Kubectl binary path is hardcoded.Unlike
k8sDeployBinandcraneBinwhich are passed as parameters, the kubectl binary is hardcoded to"kubectl". If configurability is needed (e.g., custom kubectl path or wrapper), consider accepting it as a parameter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/framework/scenario.go` around lines 37 - 38, Kubectl is hardcoded when initializing KubectlSrc and KubectlTgt; make the kubectl binary configurable by adding a kubectlBin (or similar) parameter to the constructor or factory that builds the Scenario and use that value when creating KubectlRunner{Bin: kubectlBin, Context: ...} for both KubectlSrc and KubectlTgt; update any callers to pass the new parameter (or forward an existing k8sDeployBin/craneBin if appropriate) so the runner uses the provided path/wrapper instead of the literal "kubectl".
68-82: Consider logging cleanup errors for debugging.Suppressing cleanup errors is acceptable to avoid masking test failures, but logging them would help troubleshoot resource leaks.
♻️ Optional: Log cleanup errors
func CleanupScenario(tempDir string, srcApp, tgtApp K8sDeployApp) { log.Println("Starting cleanup...") log.Printf("Removing temp dir: %s\n", tempDir) - _ = os.RemoveAll(tempDir) + if err := os.RemoveAll(tempDir); err != nil { + log.Printf("Warning: failed to remove temp dir: %v", err) + } log.Printf("Cleaning source app: %s/%s\n", srcApp.Namespace, srcApp.Name) - _ = srcApp.Cleanup() + if err := srcApp.Cleanup(); err != nil { + log.Printf("Warning: failed to cleanup source app: %v", err) + } log.Printf("Cleaning target app: %s/%s\n", tgtApp.Namespace, tgtApp.Name) - _ = tgtApp.Cleanup() + if err := tgtApp.Cleanup(); err != nil { + log.Printf("Warning: failed to cleanup target app: %v", err) + } log.Println("Cleanup completed.") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/framework/scenario.go` around lines 68 - 82, CleanupScenario currently calls os.RemoveAll and the Cleanup methods on srcApp and tgtApp but discards any returned errors; update it to log those errors for debugging: capture the error from os.RemoveAll(tempDir) and call log.Printf with context if non-nil, and likewise capture and log errors returned from srcApp.Cleanup() and tgtApp.Cleanup() (reference the function name CleanupScenario and the symbols tempDir, os.RemoveAll, srcApp.Cleanup, tgtApp.Cleanup in your changes).e2e/framework/kubectl.go (1)
81-117: Potential slice aliasing issue withappend.On lines 83 and 104, you're appending to
baseArgs. In Go, ifbaseArgshas sufficient capacity,appendmay reuse the underlying array, causing the second append to overwrite the first's data. This could cause unexpected behavior in the label-based path if the fallback loop runs.However, in this specific flow the early
return nilon line 94 prevents both paths from executing in sequence. Still, for safety and clarity, consider creating a fresh slice or usingslices.Clone.♻️ Safer slice handling
baseArgs := []string{"scale", "deployment", "--namespace", ns, "--replicas", strconv.Itoa(replicas)} if strings.TrimSpace(string(checkOut)) != "" { - args := append(baseArgs, "-l", selector) + args := append([]string{}, baseArgs...) + args = append(args, "-l", selector) if k.Context != "" {Or for the fallback:
for _, depName := range fallbackNames { - args := append(baseArgs, depName) + args := append([]string{}, baseArgs...) + args = append(args, depName) if k.Context != "" {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/framework/kubectl.go` around lines 81 - 117, The code appends to baseArgs in both the label-based branch and the fallback loop which can lead to slice aliasing; change how args is created to avoid mutating baseArgs (e.g., create a fresh slice per use like args := append([]string{}, baseArgs...), or use slices.Clone(baseArgs)) where args is built in the label-path (variable args near the first append) and inside the fallback loop (variable args in the for loop) so each exec.Command gets an independent slice and the baseArgs backing array is never shared/overwritten.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/run-e2e-tests.yml:
- Around line 25-34: The workflow step "Clone k8s-apps-deployer" uses a
hardcoded branch name 'fix-context-big' which is fragile; update that git clone
invocation to reference a pinned commit SHA or a tagged release (or document the
dependency) instead of the branch name, and in the "Create venv and install
package" step fix the ShellCheck SC2086 issue by quoting $PWD when appending to
GITHUB_PATH (use "$PWD/venv/bin" rather than $PWD/venv/bin) so path components
with spaces are handled safely; locate and update the two steps named "Clone
k8s-apps-deployer" and "Create venv and install package" in the workflow to
apply these changes.
- Around line 41-46: The GINKGO_VERSION variable used in the go install command
should be quoted to satisfy shellcheck and avoid word-splitting; update the go
install invocation that references GINKGO_VERSION (the line using go install
github.com/onsi/ginkgo/v2/ginkgo@...) to use a quoted expansion
("$GINKGO_VERSION") so the installer receives the full version string safely.
In `@e2e/framework/crane.go`:
- Line 84: Fix the typo in the log message used in the logVerboseOutput call
inside crane.go: change the string "crane tranfer-pvc" to "crane transfer-pvc"
so the log reflects the correct command; update the invocation of
logVerboseOutput("crane tranfer-pvc", out) to use the corrected message.
In `@e2e/tests/stateful_migration_test.go`:
- Around line 61-74: The PVCNamespaceMap is incorrectly built as
srcApp.Namespace:srcApp.Namespace; update the mapping to use the target
namespace so the source-to-target mapping is explicit. In the Transfer PVCs
block where TransferPVCOptions is created (variable opts), change the
PVCNamespaceMap value to format the mapping as fmt.Sprintf("%s:%s",
srcApp.Namespace, tgtApp.Namespace) so the source namespace (srcApp.Namespace)
maps to the destination namespace (tgtApp.Namespace); this fixes
TransferPVCOptions.PVCNamespaceMap usage for TransferPVC operations.
---
Nitpick comments:
In @.github/actions/setup-minikube-clusters/action.yml:
- Around line 60-66: The verification step currently hardcodes minikube profile
names ("minikube ip -p src" and "minikube ip -p tgt"), which will break when
users set SRC_PROFILE/TGT_PROFILE or choose different names; update the action
to reference configurable profile names instead (use the environment variables
SRC_PROFILE and TGT_PROFILE from hack/setup-minikube-same-network.sh or add
action inputs like src_profile and tgt_profile) and replace the hardcoded calls
in the verify step (the "Verify profiles" shell run block) to use those
variables so the verification matches the configured profiles.
- Around line 67-77: The step redundantly switches context then passes
--context=src; remove the unnecessary kubectl config use-context src line and
rely solely on the explicit --context=src in the kubectl run invocation (retain
the TGT_IP assignment and the kubectl run test-connectivity ... --context=src
... curl ... command) so the action does not perform an extra global context
change.
In @.github/workflows/run-e2e-tests.yml:
- Around line 51-57: Add the missing test flag by passing the registered
--verbose-logs flag into the ginkgo invocation so CI captures detailed test
output; update the ginkgo run command that constructs the e2e invocation (the
block invoking "ginkgo run -v e2e/tests --" with flags like --k8sdeploy-bin and
--crane-bin) to include --verbose-logs (and set it to true or the desired level)
alongside the existing flags.
In `@e2e/framework/app.go`:
- Around line 82-96: The envWithBinDir function currently appends a new
"PATH=..." entry to the env slice, causing duplicate PATH entries; modify
envWithBinDir to compute updatedPath (using filepath.Dir(bin) and
os.PathListSeparator) and then search the env slice returned by os.Environ() for
an existing entry with prefix "PATH=" and replace that element with
"PATH="+updatedPath; if no existing PATH entry is found, append the new "PATH="
entry. Ensure you still return env and keep the early return when bin has no
path separator.
In `@e2e/framework/client.go`:
- Line 84: The error string created with fmt.Errorf currently uses "No node IP
found" which violates Go convention to start errors with a lowercase letter;
update the fmt.Errorf call (the return "", fmt.Errorf(...) expression in
e2e/framework/client.go) to use a lowercase-leading message such as "no node IP
found" so the error adheres to Go error-string conventions.
In `@e2e/framework/kubectl.go`:
- Around line 81-117: The code appends to baseArgs in both the label-based
branch and the fallback loop which can lead to slice aliasing; change how args
is created to avoid mutating baseArgs (e.g., create a fresh slice per use like
args := append([]string{}, baseArgs...), or use slices.Clone(baseArgs)) where
args is built in the label-path (variable args near the first append) and inside
the fallback loop (variable args in the for loop) so each exec.Command gets an
independent slice and the baseArgs backing array is never shared/overwritten.
In `@e2e/framework/scenario.go`:
- Around line 60-65: The code returns ScenarioPaths with directories built via
string concatenation (TempDir + "/export", "/transform", "/output"); change
these to use filepath.Join from the path/filepath package to ensure
cross-platform correctness—import "path/filepath" if missing and replace
ExportDir: tempDir + "/export", TransformDir: tempDir + "/transform", OutputDir:
tempDir + "/output" with filepath.Join(tempDir, "export"),
filepath.Join(tempDir, "transform"), filepath.Join(tempDir, "output")
respectively while keeping TempDir as-is.
- Around line 37-38: Kubectl is hardcoded when initializing KubectlSrc and
KubectlTgt; make the kubectl binary configurable by adding a kubectlBin (or
similar) parameter to the constructor or factory that builds the Scenario and
use that value when creating KubectlRunner{Bin: kubectlBin, Context: ...} for
both KubectlSrc and KubectlTgt; update any callers to pass the new parameter (or
forward an existing k8sDeployBin/craneBin if appropriate) so the runner uses the
provided path/wrapper instead of the literal "kubectl".
- Around line 68-82: CleanupScenario currently calls os.RemoveAll and the
Cleanup methods on srcApp and tgtApp but discards any returned errors; update it
to log those errors for debugging: capture the error from os.RemoveAll(tempDir)
and call log.Printf with context if non-nil, and likewise capture and log errors
returned from srcApp.Cleanup() and tgtApp.Cleanup() (reference the function name
CleanupScenario and the symbols tempDir, os.RemoveAll, srcApp.Cleanup,
tgtApp.Cleanup in your changes).
In `@e2e/tests/e2e_suite_test.go`:
- Around line 22-29: In TestE2E, the line reporterConfig.Verbose = true
force-enables verbosity and overrides CLI flags; remove that assignment so
reporterConfig retains CLI-provided settings (i.e., delete or comment out
reporterConfig.Verbose = true in TestE2E) or, if intentional, replace it with a
conditional that only sets verbose when an explicit override is present (e.g.,
check an env var) to avoid clobbering Ginkgo CLI flags.
In `@e2e/tests/stateless_migration_test.go`:
- Around line 59-122: Remove or explicitly document the large commented-out
Ginkgo test block starting with the It("[MTC-129] No PVCs stage warning")
declaration: either delete the entire commented block to reduce noise, or
replace the comment with a short explanatory comment that it’s intentionally
disabled and reference a tracking issue (e.g. MTC-129) or move the test content
into an issue/PR draft. Locate the block by searching for the test title
"[MTC-129] No PVCs stage warning" (which contains symbols like
NewMigrationScenario, TransferPVC, RunCranePipelineWithChecks,
ApplyOutputToTarget) and apply the chosen cleanup so the file no longer contains
that large commented-out test.
In `@e2e/utils/utils.go`:
- Around line 50-57: HasFilesRecursively is brittle because it checks for the
literal "(no files)" in the string returned by ListFilesRecursively; instead
change ListFilesRecursively to return a structured result (e.g., ([]string,
error) or a struct with a Names []string) and update HasFilesRecursively to call
that new API and determine emptiness via len(names) > 0, then separately
format/join the names for the string return; update references to
ListFilesRecursively and the HasFilesRecursively signature/return to use the new
types (functions: ListFilesRecursively, HasFilesRecursively) so emptiness
detection no longer depends on string formatting.
In `@hack/setup-minikube-same-network.sh`:
- Around line 171-179: The patch is using a fragile hardcoded port index when
adding hostPort (see https_hostport variable and the kubectl patch calls that
target /spec/template/spec/containers/0/ports/1/hostPort and /0/), which can
break if port ordering changes; update the script to compute the correct
container port index dynamically (query
.spec.template.spec.containers[0].ports[*] for containerPort==443, convert to
0-based index) and then use that index in the JSON patch path, or replace the
JSON patch with a strategic merge/set command that matches by containerPort
instead of an array index.
- Line 37: Remove the duplicate shell debugging directive by deleting the
redundant "set -x" instance (the second occurrence) so only the initial "set -x"
remains; locate the repeated "set -x" token in the script and remove that line
to avoid the unnecessary duplicate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dda54c46-cc10-48b0-a6f9-c8a04a351f6f
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
.github/actions/setup-minikube-clusters/action.yml.github/workflows/run-e2e-tests.ymlMakefilecmd/transfer-pvc/transfer-pvc.goe2e/README.mde2e/config/config.goe2e/framework/app.goe2e/framework/client.goe2e/framework/crane.goe2e/framework/kubectl.goe2e/framework/logging.goe2e/framework/pipeline.goe2e/framework/scenario.goe2e/tests/e2e_suite_test.goe2e/tests/stateful_migration_test.goe2e/tests/stateless_migration_test.goe2e/utils/utils.gogo.modhack/setup-minikube-same-network.sh
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/tests/stateful_migration_test.go (1)
61-79: Consider extracting hardcoded ingress configuration.The
EndpointandIngressClassvalues are hardcoded. For better flexibility across different cluster configurations, consider moving these toconfigpackage.♻️ Suggested refactor
opts := TransferPVCOptions{ SourceContext: srcApp.Context, TargetContext: tgtApp.Context, PVCName: pvcName, PVCNamespaceMap: fmt.Sprintf("%s:%s", srcApp.Namespace, tgtApp.Namespace), - Endpoint: "nginx-ingress", - IngressClass: "nginx", + Endpoint: config.IngressEndpoint, + IngressClass: config.IngressClass, Subdomain: fmt.Sprintf("%s.%s.%s.nip.io", pvcName, srcApp.Namespace, tgtIP), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/tests/stateful_migration_test.go` around lines 61 - 79, The TransferPVCOptions in the stateful migration test hardcodes Endpoint and IngressClass; update the test to read these values from a central config instead. Add Endpoint and IngressClass entries to your config package (e.g., Config.Endpoint, Config.IngressClass), load or inject that config in the test setup, and replace the literals in the TransferPVCOptions construction with the config values so runner.TransferPVC(opts) uses cluster-specific settings. Ensure the config fields are documented and used everywhere else tests rely on ingress settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@e2e/tests/stateful_migration_test.go`:
- Around line 61-79: The TransferPVCOptions in the stateful migration test
hardcodes Endpoint and IngressClass; update the test to read these values from a
central config instead. Add Endpoint and IngressClass entries to your config
package (e.g., Config.Endpoint, Config.IngressClass), load or inject that config
in the test setup, and replace the literals in the TransferPVCOptions
construction with the config values so runner.TransferPVC(opts) uses
cluster-specific settings. Ensure the config fields are documented and used
everywhere else tests rely on ingress settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a59b93a4-b832-42f0-b087-c66933451d61
📒 Files selected for processing (3)
.github/workflows/run-e2e-tests.ymle2e/framework/crane.goe2e/tests/stateful_migration_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/run-e2e-tests.yml
- e2e/framework/crane.go
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
e2e-tests/utils/utils.go (1)
50-57: Fragile sentinel-string check for empty directory detection.
HasFilesRecursivelyrelies on matching the literal"(no files)"substring returned byListFilesRecursively. If that sentinel ever changes, this check silently breaks.Consider returning a boolean or file count directly from
ListFilesRecursively, or define the sentinel as a package constant.♻️ Suggested refactor using a constant
+const noFilesMessage = " (no files)" + // ListFilesRecursively returns a formatted list of files under a directory. func ListFilesRecursively(dir string) (string, error) { // ... unchanged ... if len(files) == 0 { - return " (no files)", nil + return noFilesMessage, nil } // ... } // HasFilesRecursively reports whether a directory contains any files. func HasFilesRecursively(dir string) (bool, string, error) { files, err := ListFilesRecursively(dir) if err != nil { return false, "", err } - hasFiles := !strings.Contains(files, "(no files)") + hasFiles := files != noFilesMessage return hasFiles, files, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/utils/utils.go` around lines 50 - 57, HasFilesRecursively is fragile because it detects emptiness by searching for the literal "(no files)" in the string returned by ListFilesRecursively; update the implementation by either (A) changing ListFilesRecursively to return structured data (e.g., (files string, count int, err error) or (files []string, err error)) so HasFilesRecursively can check count/length directly, or (B) if leaving the current string API, extract the sentinel into a package-level constant (e.g., NoFilesSentinel) and use that constant in HasFilesRecursively when calling ListFilesRecursively, and update any tests that assert the old literal to reference the constant; make sure to update the signature and callers of ListFilesRecursively if you choose option A so callers use the new count/length check instead of substring matching.e2e-tests/tests/stateless_migration_test.go (1)
59-122: Remove or track commented-out test code.This large block of commented-out test code clutters the file. Either delete it or extract it to a separate issue/TODO for future implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/tests/stateless_migration_test.go` around lines 59 - 122, The large commented-out test block starting with It("[MTC-129] No PVCs stage warning") should be removed or tracked; either delete the entire commented section or replace it with a single short TODO comment that references an issue/track ID and briefly states why the test was removed (e.g., "TODO: restore MTC-129 test — tracked in ISSUE-123"), so the file is not cluttered; locate the block by searching for the test name It("[MTC-129] No PVCs stage warning") or symbols like NewMigrationScenario, RunCranePipelineWithChecks, TransferPVCOptions/TransferPVC and remove or replace the commented code accordingly.e2e-tests/framework/crane.go (1)
8-13: RemoveTargetContextfield or initialize it inNewMigrationScenario.
CraneRunner.TargetContextis defined but never assigned—NewMigrationScenarioonly setsBinandSourceContext(seescenario.go:39-42). No method reads this field;TransferPVCuses context values from itsTransferPVCOptionsparameter instead. Either remove the unused field or initialize it if future use is planned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/framework/crane.go` around lines 8 - 13, CraneRunner.TargetContext is declared but never assigned; update NewMigrationScenario to initialize TargetContext (set it from the scenario or a parameter) if you expect it to be used, or remove the TargetContext field from the CraneRunner struct to avoid dead state; search for CraneRunner, NewMigrationScenario and TransferPVC to decide whether future calls expect CraneRunner.TargetContext and either initialize it in NewMigrationScenario or delete the unused field accordingly.e2e-tests/tests/stateful_migration_test.go (1)
34-36: Remove duplicate source scale-down call.Line 35 is redundant because
PrepareSourceApp(...)already scales the source deployment to0(e2e-tests/framework/pipeline.go, Line 50).♻️ Proposed simplification
- By("Scale src app to 0 replicas") - Expect(kubectlSrc.ScaleDeployment(srcApp.Namespace, srcApp.Name, 0)).NotTo(HaveOccurred())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-tests/tests/stateful_migration_test.go` around lines 34 - 36, Remove the redundant scale-down call that duplicates PrepareSourceApp's behavior: delete the call Expect(kubectlSrc.ScaleDeployment(srcApp.Namespace, srcApp.Name, 0)).NotTo(HaveOccurred()) (and its surrounding By("Scale src app to 0 replicas") message) from stateful_migration_test.go since PrepareSourceApp already scales the source deployment to 0; keep srcApp, PrepareSourceApp, and kubectlSrc references intact and ensure no other logic depends on the duplicate call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-tests/framework/app.go`:
- Around line 83-95: The envWithBinDir function currently appends a new
"PATH=..." entry causing duplicate PATH env vars; modify envWithBinDir to
iterate over the env slice (returned by os.Environ()), detect an entry starting
with "PATH=" and replace that element with the new PATH value (constructed from
binDir + os.PathListSeparator + existing PATH when non-empty), and only append
"PATH=..." if no existing PATH entry is found; keep the same logic for computing
binDir and updatedPath but update the existing env entry instead of blindly
appending.
In `@e2e-tests/framework/client.go`:
- Line 84: Change the error string created by fmt.Errorf("No node IP found") to
follow Go conventions (start with a lowercase letter and no punctuation), e.g.
replace that expression with fmt.Errorf("no node IP found") (or errors.New("no
node IP found")) in the function containing the return to ensure the returned
error message is idiomatic.
In `@e2e-tests/framework/scenario.go`:
- Around line 69-79: CleanupScenario currently swallows errors from os.RemoveAll
and the K8sDeployApp Cleanup methods; change it to propagate failures by
returning an error (update the signature of CleanupScenario to return error) or
at minimum aggregate and log them as non-ignored failures. Specifically capture
the results of os.RemoveAll(tempDir), srcApp.Cleanup(), and tgtApp.Cleanup(),
collect any non-nil errors (e.g., via a slice or a multi-error helper), and
return a combined error (or a single wrapped error) so callers can detect leaked
resources; ensure callers are updated to handle the new error return if you
choose to change the signature.
In `@e2e-tests/tests/stateful_migration_test.go`:
- Around line 17-33: The test currently calls ScaleDeployment redundantly after
PrepareSourceApp; remove the extra ScaleDeployment call and rely on
PrepareSourceApp to scale the source deployment to 0 replicas. Locate the test's
use of PrepareSourceApp (and the subsequent ScaleDeployment call that references
srcApp and kubectlSrc) and delete the ScaleDeployment invocation so the scenario
only uses PrepareSourceApp to perform the scaling.
---
Nitpick comments:
In `@e2e-tests/framework/crane.go`:
- Around line 8-13: CraneRunner.TargetContext is declared but never assigned;
update NewMigrationScenario to initialize TargetContext (set it from the
scenario or a parameter) if you expect it to be used, or remove the
TargetContext field from the CraneRunner struct to avoid dead state; search for
CraneRunner, NewMigrationScenario and TransferPVC to decide whether future calls
expect CraneRunner.TargetContext and either initialize it in
NewMigrationScenario or delete the unused field accordingly.
In `@e2e-tests/tests/stateful_migration_test.go`:
- Around line 34-36: Remove the redundant scale-down call that duplicates
PrepareSourceApp's behavior: delete the call
Expect(kubectlSrc.ScaleDeployment(srcApp.Namespace, srcApp.Name,
0)).NotTo(HaveOccurred()) (and its surrounding By("Scale src app to 0 replicas")
message) from stateful_migration_test.go since PrepareSourceApp already scales
the source deployment to 0; keep srcApp, PrepareSourceApp, and kubectlSrc
references intact and ensure no other logic depends on the duplicate call.
In `@e2e-tests/tests/stateless_migration_test.go`:
- Around line 59-122: The large commented-out test block starting with
It("[MTC-129] No PVCs stage warning") should be removed or tracked; either
delete the entire commented section or replace it with a single short TODO
comment that references an issue/track ID and briefly states why the test was
removed (e.g., "TODO: restore MTC-129 test — tracked in ISSUE-123"), so the file
is not cluttered; locate the block by searching for the test name It("[MTC-129]
No PVCs stage warning") or symbols like NewMigrationScenario,
RunCranePipelineWithChecks, TransferPVCOptions/TransferPVC and remove or replace
the commented code accordingly.
In `@e2e-tests/utils/utils.go`:
- Around line 50-57: HasFilesRecursively is fragile because it detects emptiness
by searching for the literal "(no files)" in the string returned by
ListFilesRecursively; update the implementation by either (A) changing
ListFilesRecursively to return structured data (e.g., (files string, count int,
err error) or (files []string, err error)) so HasFilesRecursively can check
count/length directly, or (B) if leaving the current string API, extract the
sentinel into a package-level constant (e.g., NoFilesSentinel) and use that
constant in HasFilesRecursively when calling ListFilesRecursively, and update
any tests that assert the old literal to reference the constant; make sure to
update the signature and callers of ListFilesRecursively if you choose option A
so callers use the new count/length check instead of substring matching.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53d2beba-908e-4af2-b7c8-4ab32cb70fb7
📒 Files selected for processing (14)
.github/workflows/run-e2e-tests.ymle2e-tests/README.mde2e-tests/config/config.goe2e-tests/framework/app.goe2e-tests/framework/client.goe2e-tests/framework/crane.goe2e-tests/framework/kubectl.goe2e-tests/framework/logging.goe2e-tests/framework/pipeline.goe2e-tests/framework/scenario.goe2e-tests/tests/e2e_suite_test.goe2e-tests/tests/stateful_migration_test.goe2e-tests/tests/stateless_migration_test.goe2e-tests/utils/utils.go
✅ Files skipped from review due to trivial changes (2)
- e2e-tests/README.md
- .github/workflows/run-e2e-tests.yml
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
midays
requested changes
Mar 23, 2026
Contributor
midays
left a comment
midays
left a comment
There was a problem hiding this comment.
Nicely done! Left 2 comments, otherwise looks great
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Resolve go.sum conflict: keep pvc-transfer v0.0.0-20220810121213 (main migtools#176). Made-with: Cursor Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Resolve go.mod/go.sum: take main dependency updates (Go 1.25, k8s 0.35, OpenShift API bump, cobra/kustomize) and retain ginkgo/gomega for e2e. Regenerate go.sum with go mod tidy. Made-with: Cursor Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
Signed-off-by: M Sajid Mansoori <mmansoor@redhat.com>
msajidmansoori12
force-pushed
the
test-setup-minikube
branch
from
171b584 to
9c9c6b8
Compare
This was referenced Mar 26, 2026
This was referenced Apr 7, 2026
This was referenced Apr 16, 2026
Merged
This was referenced Apr 20, 2026
This was referenced Apr 21, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR introduces a dedicated E2E testing framework for automating crane migration scenarios.
What’s included
Adds reusable E2E test infrastructure (shared runners, scenario setup, pipeline helpers, cleanup, and utilities).
Adds initial automated E2E scenarios:
one stateless migration test
one stateful migration test
Adds e2e/README.md documenting the framework structure, execution flow, and how to run/extend tests.
Adds a GitHub Action to set up Minikube source/target clusters on the same Docker network for crane transfer-pvc workflows.
Adds a setup script (invocable via make) to bootstrap the required multi-cluster Minikube networking environment.
Adds an E2E workflow that provisions the environment and runs the current E2E suite in CI.
Summary by CodeRabbit
Release Notes
New Features
Chores