make building image and setting infra optional - #598
Conversation
Signed-off-by: Lionel Villard <villard@us.ibm.com>
There was a problem hiding this comment.
Pull request overview
This pull request refactors test utility functions by moving llm-d-sim resource creation functions from test/utils/e2eutils.go to a new dedicated package test/utils/resources. Additionally, it introduces optional environment variables for the e2e-saturation test suite to skip building images and setting up infrastructure, enabling faster test iterations during development.
Changes:
- Moved llm-d-sim deployment, service, and ServiceMonitor creation functions to
test/utils/resources/llmdsim.go - Added support for skipping image builds (
IMAGE_BUILD_SKIP) and infrastructure setup/teardown (INFRA_SETUP_SKIP,INFRA_TEARDOWN_SKIP) in e2e-saturation tests - Updated all test files to import and use the new
resourcespackage
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils/resources/llmdsim.go | New file containing three helper functions for creating llm-d-sim Kubernetes resources |
| test/utils/e2eutils.go | Removed llm-d-sim resource creation functions and their import dependencies |
| test/e2e/e2e_test.go | Updated imports and function calls to use the new resources package |
| test/e2e-saturation-based/e2e_saturation_test.go | Updated imports and function calls to use the new resources package |
| test/e2e-saturation-based/e2e_saturation_suite_test.go | Added environment variables for optional test setup steps with conditional logic |
| internal/controller/variantautoscaling_controller_test.go | Updated imports and function calls to use the new resources package |
| Makefile | Updated documentation for new environment variables |
| // These variables are useful if CertManager is already installed, avoiding | ||
| // re-installation and conflicts. | ||
| // - IMAGE_BUILD_SKIP=true: Skips building the WVA docker image during test setup. | ||
| // - DEPLOY_INFRA_SKIP=true: Skips deploying the WVA controller manager during test setup. |
There was a problem hiding this comment.
The comment references DEPLOY_INFRA_SKIP but the actual environment variable used in the code (line 44) is INFRA_SETUP_SKIP. This inconsistency could confuse users.
| // - DEPLOY_INFRA_SKIP=true: Skips deploying the WVA controller manager during test setup. | |
| // - INFRA_SETUP_SKIP=true: Skips deploying the WVA controller manager during test setup. |
| _, err = utils.Run(cmd) | ||
| ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to build the manager(Operator) image") | ||
| } else { | ||
| _, _ = fmt.Fprintf(GinkgoWriter, "Skipping WVA image build (WVA_IMAGE_BUILD_SKIP=true)\n") |
There was a problem hiding this comment.
The message references WVA_IMAGE_BUILD_SKIP but the actual environment variable used in the code (line 43) is IMAGE_BUILD_SKIP. The variable names should be consistent.
| _, _ = fmt.Fprintf(GinkgoWriter, "Skipping WVA image build (WVA_IMAGE_BUILD_SKIP=true)\n") | |
| _, _ = fmt.Fprintf(GinkgoWriter, "Skipping WVA image build (IMAGE_BUILD_SKIP=true)\n") |
See below.