Conversation
- Updated `golang.org/x/mod` to v0.32.0 and other indirect dependencies. - Introduced end-to-end tests for the `kubectl migrate export` command, including basic export functionality and error handling for invalid namespaces. - Added utility functions for managing Kubernetes namespaces and deployments. - Created a README for the export command tests outlining test scenarios and prerequisites. Signed-off-by: mohamed idays <midays@redhat.com>
midays
changed the title
📝 WalkthroughWalkthroughAdds a Ginkgo/Gomega end-to-end testing framework: new test suites for the export command, test utilities for command execution, cluster management and config loading, test wiring/setup, test documentation, a config template, .gitignore updates, and Go module dependency bumps. Changes
Sequence Diagram(s)sequenceDiagram
participant Ginkgo as Ginkgo Test Runner
participant Setup as tests.SetupClusters
participant Config as tests/utils.LoadConfig
participant ClusterSrc as SrcCluster (kubectl context)
participant Kubectl as kubectl CLI
participant KubeAPI as Kubernetes API
Ginkgo->>Setup: BeforeSuite -> SetupClusters()
Setup->>Config: load config.yaml
Config-->>Setup: Config (source/target contexts)
Setup->>ClusterSrc: NewClusterWithContext(source.name, source.context)
ClusterSrc->>Kubectl: kubectl --context=<source.context> cluster-info
Kubectl->>KubeAPI: request cluster-info
KubeAPI-->>Kubectl: cluster-info response
Kubectl-->>ClusterSrc: return result
ClusterSrc-->>Setup: connectivity OK
Ginkgo->>ClusterSrc: Run test -> CreateNamespace, DeployApp, RunKubectl(...)
ClusterSrc->>Kubectl: kubectl --context=<source.context> apply/create/get
Kubectl->>KubeAPI: CRUD operations
KubeAPI-->>Kubectl: responses
Kubectl-->>Ginkgo: command results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used📓 Path-based instructions (1)**/*⚙️ CodeRabbit configuration file
Files:
🧬 Code graph analysis (2)tests/utils/helper_functions.go (2)
tests/utils/cluster.go (1)
⏰ 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). (1)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/e2e/export/export_test.go`:
- Line 10: Update the incorrect import path in the export_test.go import block:
replace "github.com/konveyor-ecosystem/kubectl-migrate/test/utils" with
"github.com/konveyor-ecosystem/kubectl-migrate/tests/utils" so the test uses the
correct utilities package (adjust the import string in the file containing the
Test export code).
In `@tests/utils/cluster.go`:
- Around line 70-84: Replace the custom string search implementations (functions
contains and findSubstring) with the standard library: remove findSubstring and
either delete contains and replace all calls to contains(s, substr) with
strings.Contains(s, substr) or keep contains as a thin wrapper that returns
strings.Contains(s, substr); also add the "strings" import. Update any call
sites that referenced contains to use strings.Contains (or the new wrapper) so
the code uses the standard, idiomatic implementation.
- Around line 7-23: The CheckKindCluster function accepts clusterName but never
uses it; update CheckKindCluster to inspect the output of
RunCommand("kind","get","clusters") (e.g., result.Output) and verify
strings.Contains(result.Output, clusterName), returning a clear error if the
named cluster is not present, then proceed to RunKubectl("cluster-info") as
before; add the "strings" import and keep existing error handling using
result.Error/result.Stderr and the RunCommand/RunKubectl helpers.
🧹 Nitpick comments (7)
tests/e2e/export/readme.md (1)
22-48: Documentation describes tests that are not yet implemented.The readme documents three test scenarios (Basic Export, Invalid Namespace, Custom Kubeconfig) and test data (nginx, redis, configmaps), but
export_test.goonly implements the Basic Export test with a single nginx deployment. Consider either:
- Implementing the documented test scenarios, or
- Updating the documentation to reflect the current implementation and adding TODO comments for planned tests.
This mismatch could confuse contributors about test coverage.
go.mod (1)
72-73: Ginkgo/Gomega are direct test dependencies but marked as indirect.These packages are imported directly in
tests/e2e_suite_test.goandtests/e2e/export/export_test.go. While the current setup works becausego mod tidyhandles this based on package location, explicitly listing them as direct dependencies in a separaterequireblock with a// testcomment would improve clarity.This is a minor suggestion for maintainability.
tests/e2e/export/export_test.go (3)
32-35: Static namespace name may cause parallel test conflicts.Using a hardcoded namespace
test-export-nscan cause test failures when multiple test runs execute in parallel on the same cluster. Consider generating a unique namespace name per test run.♻️ Proposed fix using unique namespace names
+import ( + "os" + "path/filepath" + "fmt" + "time" ... +) BeforeEach(func() { - testNamespace = "test-export-ns" + testNamespace = fmt.Sprintf("test-export-ns-%d", time.Now().UnixNano()) appName = "test-nginx"
45-50: Consider waiting for deployment readiness before export.The test deploys an app and immediately proceeds without waiting for the deployment to become ready. While
kubectl migrate exportmight not require ready pods, it's good practice to ensure resources are fully created. Additionally, the deployment might not export properly if the Kubernetes API hasn't fully reconciled the resources.♻️ Add deployment readiness wait
Add a wait after deployment in
DeployTestAppor here:// After deploying test app, wait for it to be ready err = utils.DeployTestApp(testNamespace, appName) Expect(err).NotTo(HaveOccurred(), "Should deploy test app successfully") // Wait for deployment to be ready (add this) result := utils.RunKubectl("wait", "--for=condition=available", "deployment/"+appName, "-n", testNamespace, "--timeout=60s") Expect(result.Success()).To(BeTrue(), "Deployment should become ready")
59-76: Cleanup failures in AfterEach should not fail the test.Using
ExpectinAfterEachcan cause misleading test failures when cleanup fails but the actual test passed. Consider logging cleanup errors instead or usingEventuallywith a timeout for namespace deletion (which can take time).♻️ More resilient cleanup
AfterEach(func() { GinkgoWriter.Println("Cleaning up...") // Remove export directory if exportDir != "" { - err := os.RemoveAll(exportDir) - Expect(err).NotTo(HaveOccurred(), "Should remove export directory") + if err := os.RemoveAll(exportDir); err != nil { + GinkgoWriter.Printf("Warning: failed to remove export directory: %v\n", err) + } GinkgoWriter.Println("✓ Export directory removed") } // Delete test namespace if testNamespace != "" { - err := utils.DeleteNamespace(testNamespace) - Expect(err).NotTo(HaveOccurred(), "Should delete namespace") + if err := utils.DeleteNamespace(testNamespace); err != nil { + GinkgoWriter.Printf("Warning: failed to delete namespace: %v\n", err) + } GinkgoWriter.Printf("✓ Namespace %s deleted\n", testNamespace) } })tests/utils/command.go (1)
18-45: Consider adding timeout support for command execution.Commands can hang indefinitely if the underlying process never completes. For E2E tests, this could cause test suites to hang. Consider adding a context-based timeout option.
♻️ Add timeout support using context
+import ( + "context" + "time" ... +) +// RunCommandWithTimeout executes a command with a timeout +func RunCommandWithTimeout(timeout time.Duration, name string, args ...string) CommandResult { + ctx, cancel := context.WithTimeout(context.Background(), timeout) + defer cancel() + + cmd := exec.CommandContext(ctx, name, args...) + // ... rest of implementation +} // RunCommand executes a shell command and returns the result func RunCommand(name string, args ...string) CommandResult { - cmd := exec.Command(name, args...) + // Default 5 minute timeout for E2E commands + return RunCommandWithTimeout(5*time.Minute, name, args...) +}tests/utils/cluster.go (1)
48-51: Avoid usinglatesttag for test images.Using
nginx:latestcan lead to non-reproducible tests if the image changes. Pin to a specific version for consistent behavior.♻️ Use a pinned image version
result := RunKubectl("create", "deployment", appName, - "--image=nginx:latest", + "--image=nginx:1.25-alpine", "-n", namespace)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
go.modtests/e2e/export/export_test.gotests/e2e/export/readme.mdtests/e2e_suite_test.gotests/utils/cluster.gotests/utils/command.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: This is a Kubernetes migration tool. Focus on Kubernetes best practices and Go idioms.
Pay special attention to error handling and resource cleanup.
Ensure all Kubernetes API calls are properly authenticated and authorized.
Files:
tests/e2e_suite_test.gotests/e2e/export/readme.mdtests/utils/command.gotests/e2e/export/export_test.gotests/utils/cluster.gogo.mod
🧬 Code graph analysis (2)
tests/e2e/export/export_test.go (2)
tests/utils/cluster.go (4)
CheckKindCluster(8-23)CreateNamespace(26-35)DeployTestApp(47-68)DeleteNamespace(38-44)tests/utils/command.go (1)
RunCommand(19-45)
tests/utils/cluster.go (1)
tests/utils/command.go (2)
RunCommand(19-45)RunKubectl(48-50)
🔇 Additional comments (2)
tests/e2e_suite_test.go (1)
1-13: LGTM!Standard Ginkgo test suite bootstrap following established conventions.
tests/utils/command.go (1)
52-55: LGTM!The
Success()method correctly checks bothExitCode == 0andError == nil, handling edge cases where the command might fail to execute entirely.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
- Updated `.gitignore` to include test configuration files. - Removed obsolete `e2e_suite_test.go` file. - Added comprehensive `readme.md` for the testing framework, detailing structure, prerequisites, and best practices. - Introduced `shared.go` for global cluster setup and management. - Created `export_suite_test.go` and `export_test.go` for export command tests, implementing a structured approach to testing. - Enhanced utility functions for cluster management and configuration loading. Signed-off-by: [Your Name] <[Your Email]> Signed-off-by: mohamed idays <midays@redhat.com>
midays
changed the title
tests/ infra
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@tests/readme.md`:
- Around line 15-33: Update the markdown fenced code blocks that show the
directory tree and the sample output to include a language identifier (use
"text") so markdownlint stops flagging them; specifically, change the opening
fences for the directory tree block (the block containing the tests/ tree) and
the sample output block (the CURRENT NAME CLUSTER listing) from ``` to ```text
in tests/readme.md around the snippets used by the README examples.
In `@tests/shared.go`:
- Around line 3-7: The test suite currently treats a missing tests/config.yaml
as a hard failure; update the test setup in tests/shared.go (the BeforeSuite or
init that loads config) to first check for the presence of "tests/config.yaml"
(using os.Stat or os.IsNotExist) and if it is missing call Ginkgo's Skip (Skip
or Skipf) so e2e setup is skipped instead of failing; ensure any opened
resources are only created after the file check so cleanup logic remains
correct.
In `@tests/utils/cluster.go`:
- Around line 77-96: In Cluster.DeployApp, if creating the service fails after a
successful deployment (i.e., the second RunKubectl call in DeployApp fails),
ensure you clean up the previously created deployment by invoking
c.RunKubectl("delete", "deployment", appName, "-n", namespace) (or the cluster's
deletion helper) before returning the error; if the delete itself errors, log or
append that delete error but preserve and return the original service-creation
failure to avoid leaving the deployment behind and to aid debugging.
🧹 Nitpick comments (1)
tests/utils/config.go (1)
86-120: Fail fast on unknown cluster types.Defaulting to generic on typos can hide misconfiguration and lead to unexpected behavior.
As per coding guidelines, Pay special attention to error handling and resource cleanup.✅ Suggested change
func (c *Config) CreateSourceCluster() (*Cluster, error) { - clusterType := parseClusterType(c.Clusters.Source.Type) + clusterType, err := parseClusterType(c.Clusters.Source.Type) + if err != nil { + return nil, fmt.Errorf("invalid source cluster type: %w", err) + } @@ func (c *Config) CreateTargetCluster() (*Cluster, error) { - clusterType := parseClusterType(c.Clusters.Target.Type) + clusterType, err := parseClusterType(c.Clusters.Target.Type) + if err != nil { + return nil, fmt.Errorf("invalid target cluster type: %w", err) + } @@ -func parseClusterType(typeStr string) ClusterType { +func parseClusterType(typeStr string) (ClusterType, error) { switch typeStr { case "kind": - return ClusterTypeKind + return ClusterTypeKind, nil case "minikube": - return ClusterTypeMinikube + return ClusterTypeMinikube, nil case "openshift": - return ClusterTypeOpenShift + return ClusterTypeOpenShift, nil + case "generic": + return ClusterTypeGeneric, nil default: - return ClusterTypeGeneric + return "", fmt.Errorf("unknown cluster type %q", typeStr) } }Also applies to: 122-134
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignoretests/e2e/export/export_suite_test.gotests/e2e/export/export_test.gotests/readme.mdtests/shared.gotests/utils/cluster.gotests/utils/config.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/e2e/export/export_test.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*
⚙️ CodeRabbit configuration file
**/*: This is a Kubernetes migration tool. Focus on Kubernetes best practices and Go idioms.
Pay special attention to error handling and resource cleanup.
Ensure all Kubernetes API calls are properly authenticated and authorized.
Files:
tests/readme.mdtests/shared.gotests/utils/config.gotests/utils/cluster.gotests/e2e/export/export_suite_test.go
🧬 Code graph analysis (4)
tests/shared.go (2)
tests/utils/cluster.go (1)
Cluster(18-23)tests/utils/config.go (1)
GetConfig(37-44)
tests/utils/config.go (1)
tests/utils/cluster.go (7)
Cluster(18-23)NewClusterWithContext(26-38)ClusterType(8-8)ClusterTypeKind(11-11)ClusterTypeMinikube(12-12)ClusterTypeOpenShift(13-13)ClusterTypeGeneric(14-14)
tests/utils/cluster.go (1)
tests/utils/command.go (2)
RunKubectl(48-50)CommandResult(11-16)
tests/e2e/export/export_suite_test.go (1)
tests/shared.go (1)
SetupClusters(17-51)
🪛 GitHub Actions: CI
tests/shared.go
[error] 25-25: Failed to load config.yaml: open tests/config.yaml: no such file or directory. (Command: make test-unit)
🪛 markdownlint-cli2 (0.20.0)
tests/readme.md
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 74-74: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (7)
.gitignore (1)
36-43: LGTM for new test artifact ignores.These patterns keep generated test outputs and configs out of version control.
tests/utils/cluster.go (4)
25-37: Constructor setup is clear.The OpenShift flag derivation keeps the struct intent explicit.
40-55: Context-scoped kubectl invocation looks good.Prepending the context and surfacing a clear connectivity error is solid.
57-75: Namespace create/delete flow is straightforward.Handling “already exists” and using
--ignore-not-foundkeeps operations idempotent for tests.
101-109: Helper methods are concise.The accessors do what they say without extra complexity.
tests/utils/config.go (1)
36-43: Lazy config load is clear.The
sync.Oncegate keeps repeated tests from re-reading the file.tests/e2e/export/export_suite_test.go (1)
12-20: Suite wiring looks good.Standard Ginkgo setup and shared cluster initialization are cleanly integrated.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| ## Directory Structure | ||
| ``` | ||
| tests/ | ||
| ├── config.yaml # Cluster configuration (git-ignored) | ||
| ├── config.yaml.template # Template for cluster configuration | ||
| ├── shared.go # Global cluster variables and setup function | ||
| ├── e2e_suite_test.go # Main test suite (optional) | ||
| ├── utils/ # Testing utilities | ||
| │ ├── cluster.go # Cluster management (Cluster struct) | ||
| │ ├── command.go # Command execution utilities | ||
| │ └── config.go # Configuration loading | ||
| └── e2e/ # End-to-end test suites | ||
| ├── export/ | ||
| │ ├── export_suite_test.go # Export test suite setup | ||
| │ ├── export_test.go # Export test cases | ||
| │ └── README.md # Export test documentation | ||
| └── transform/ | ||
| └── ... | ||
| ``` |
There was a problem hiding this comment.
Add languages to fenced code blocks.
Markdownlint flags the directory tree and sample output fences as missing a language.
✍️ Suggested fix
-```
+```text
tests/
├── config.yaml # Cluster configuration (git-ignored)
@@
-```
+```text
CURRENT NAME CLUSTER
* kind-src-cluster kind-src-cluster
kind-tgt-cluster kind-tgt-cluster
-```
+```Also applies to: 73-78
🧰 Tools
🪛 markdownlint-cli2 (0.20.0)
[warning] 16-16: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In `@tests/readme.md` around lines 15 - 33, Update the markdown fenced code blocks
that show the directory tree and the sample output to include a language
identifier (use "text") so markdownlint stops flagging them; specifically,
change the opening fences for the directory tree block (the block containing the
tests/ tree) and the sample output block (the CURRENT NAME CLUSTER listing) from
``` to ```text in tests/readme.md around the snippets used by the README
examples.
- Introduced a new `config.yaml.template` for easier test configuration setup. - Updated `readme.md` to remove cluster type references, simplifying the configuration process. - Refactored cluster creation logic in `utils/config.go` to streamline cluster initialization. - Removed unnecessary cluster type handling from the `Cluster` struct and related functions. Signed-off-by: [Your Name] <[Your Email]> Signed-off-by: mohamed idays <midays@redhat.com>
- Removed the `DeployApp` function from `cluster.go` to streamline cluster management. - Moved the `contains` helper function to a new file `helper_functions.go` for better organization and reusability. - Updated the `GetPods` and `String` methods in `cluster.go` to maintain functionality after the refactor. Signed-off-by: [Your Name] <[Your Email]> Signed-off-by: mohamed idays <midays@redhat.com>
- Implemented a check in `setup.go` to verify the presence of `config.yaml` before running end-to-end tests. - If the configuration file is missing, tests will be skipped with a detailed message on how to create the necessary configuration. Signed-off-by: [Your Name] <[Your Email]> Signed-off-by: mohamed idays <midays@redhat.com>
- Updated `setup.go` to improve the handling of missing `config.yaml` by providing a more informative skip message. - Refactored the configuration loading logic to differentiate between file absence and other errors. - Ensured that the cleanup process in `export_test.go` is correctly executed by uncommenting the namespace deletion. Signed-off-by: [Your Name] <[Your Email]> Signed-off-by: mohamed idays <midays@redhat.com>
| // RunKubectl runs a kubectl command against this cluster | ||
| func (c *Cluster) RunKubectl(args ...string) CommandResult { | ||
| // Prepend --context flag to use this cluster | ||
| fullArgs := append([]string{"--context", c.Context}, args...) |
There was a problem hiding this comment.
Hi @midays, I have a suggestion :
The --context flag needs to be appended rather than prepended. kubectl plugins don't accept global flags before the plugin name:
❌ kubectl --context kind-src migrate export ...
→ Error: flags cannot be placed before plugin name: --context
✅ kubectl migrate export ... --context kind-src
→ Works correctly
Fix:
fullArgs := append(args, "--context", c.Context)
aufi
left a comment
aufi
left a comment
There was a problem hiding this comment.
Thanks for the PR @midays!
I left some comments, mostly for discussion or to make me better understanding objectives of E2E test suite here.
Main points I'd suggest are:
- Consider Cluster vs. Context naming (theoretically not a significant problem, but better use the same (and descriptive) naming across the project)
- Consider kubectl cmd golang wrapper vs. k8s client-go (https://github.com/kubernetes/client-go) to setup test env ()
- Add test setup/cleanup (e.g. automatically create namespace (context), load existing sample resources, tests, deleting namespaces etc.; not sure if config.yaml is needed)
- Add the
testtarget to Makefile along with https://github.com/konveyor-ecosystem/kubectl-migrate/blob/main/Makefile#L68 to execute E2E test suite - Add its exection into upstream CI/github workflow
- (Not saying everything needs to come with this PR and I appreciate examples in README)
I migh have too developer-ish point of view, please keep me honest if some points don't make sense from QE perspective.
|
|
||
| ## Test Data | ||
|
|
||
| - Sample deployments: nginx, redis |
There was a problem hiding this comment.
Just a question, do these deployments come from sample resources? (not sure about redis, maybe reference to sample resources directly).
|
|
||
| // RunKubectl is a convenience wrapper for kubectl commands | ||
| func RunKubectl(args ...string) CommandResult { | ||
| return RunCommand("kubectl", args...) |
There was a problem hiding this comment.
Files in utils directory look to be a wrapper for kubectl command execution, thinking if it is better than using https://github.com/kubernetes/client-go that allows do the same, but as a lib, not separate command (providing methods like client.CoreV1().Namespaces().Create()).
The CLI wrapper code is probably more readable than the lib, but I'd guess it to be more fragile/less reliable. Anyway, defferring to your QE experience.
| type Config struct { | ||
| Clusters struct { | ||
| Source ClusterConfig `yaml:"source"` | ||
| Target ClusterConfig `yaml:"target"` |
There was a problem hiding this comment.
Nit, IIRC it was more common use Source&Dest (destination). No problem with Target too, just better use consistent naming.
| // Cluster represents a Kubernetes cluster with its context and type | ||
| type Cluster struct { | ||
| Name string | ||
| Context string |
There was a problem hiding this comment.
Thinking on Cluster vs. Context. We will most likely migrate resources between contexts (regardless k8s cluster they run on). In kubectl CLI, there is a context name (string), that points to kube config file with more details on the context. It depends if we want store context names or if it is better have Cluster struct around it.
|
|
||
| Avoid conflicts by using unique names: | ||
| ```go | ||
| testNs := fmt.Sprintf("test-%d", GinkgoRandomSeed()) |
| // CreateCluster creates a Cluster instance from a ClusterConfig | ||
| func (c *Config) CreateCluster(clusterConfig ClusterConfig) (*Cluster, error) { | ||
|
|
||
| cluster := NewClusterWithContext( |
There was a problem hiding this comment.
Not sure about the method NewClusterWithContext, that just populates a struct https://github.com/konveyor-ecosystem/kubectl-migrate/pull/51/changes#diff-823b3cdd6ae7e93a5224c573a1a47c832e114bd19f647b97849b921af2ad6b81R14
| target: | ||
| name: "tgt_cluster" # Friendly name for the cluster | ||
| context: "kind-tgt-cluster" # Context name from 'kubectl config get-contexts' | ||
|
|
There was a problem hiding this comment.
Just thinking if hardcoding a YAML config file is the best way, other parts of code recommend dynamic/unique names creation for namespaces (which looks great to me, mentioned in README), hardcoding it into file looks to add an unnecesarry complexity to me. Please correct me if it is better from QE perspective.
3 participants
golang.org/x/modto v0.32.0 and other indirect dependencies.kubectl migrate exportcommand, including basic export functionality and error handling for invalid namespaces.Summary by CodeRabbit
Tests
Documentation
Chores