Repository navigation
test: add E2E matrix test for deploy -e flag (fixes #3514) - #3806
knative-prow[bot] merged 2 commits into
Conversation
knative-prow
Bot
added
size/L
|
Hi @Itx-Psycho0. Thanks for your PR. I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Hi @lkingland sir, I've implemented the E2E matrix test you suggested for issue #3514. The test creates a function that echoes back environment variables, deploys it with The test covers all 7 runtimes (go, python, node, typescript, rust, quarkus, springboot), all 3 builders (host, pack, s2i), and both templates (http, cloudevents). Each language has its own implementation for reading and returning the environment variables. This should help us verify whether the Would appreciate your review when you get a chance. Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #3806 +/- ##
===========================================
- Coverage 57.06% 45.41% -11.65%
===========================================
Files 181 200 +19
Lines 21145 23426 +2281
===========================================
- Hits 12067 10640 -1427
- Misses 7855 11843 +3988
+ Partials 1223 943 -280
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
lkingland
added
ok-to-test
|
Some check if failing. |
|
This Pull Request is stale because it has been open for 90 days with |
|
/reopen |
|
@Itx-Psycho0: Reopened this PR. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Add comprehensive E2E matrix test to verify that environment variables passed via `func deploy -e KEY=VALUE` are correctly applied to deployed functions across all supported builders and runtimes. The test: - Creates a function that echoes back environment variables - Deploys it with `func deploy -e TEST_VAR=test_value -e ANOTHER_VAR=another_value` - Invokes the function and verifies env vars are present - Runs for all builder ?? runtime ?? template combinations This addresses the lack of test coverage for the -e flag on deploy, which was identified as the root cause of issue knative#3514. Implementation: - Added TestMatrix_Deploy_Envs() function in e2e/e2e_matrix_deploy_envs_test.go - Added createEnvEchoFunction() helper for language-specific implementations - Supports all 7 runtimes: go, python, node, typescript, rust, quarkus, springboot - Supports all 3 builders: host, pack, s2i - Supports both templates: http, cloudevents The test will run in CI when FUNC_E2E_MATRIX=true is set, verifying that the -e flag works correctly across all supported combinations and preventing future regressions. Fixes knative#3514
The first revision wrote each language's echo to a path the template does not build, and used handler shapes the current templates no longer have. The deployed container therefore kept answering "OK", and waitFor treated that as ready. Overwrite the http template's real handler, and the template tests that assert the stock body, so the image serves ENV_ECHO. Skip cloudevents: those handlers are a different contract, and http is enough to prove deploy -e reached the process. waitFor matches ENV_ECHO, then the test still requires TEST_VAR=test_value and ANOTHER_VAR=another_value. A marker without those values is knative#3514. Leave that assertion in place. Issue knative#3514
ff84bf8 to
3664c14
Compare
knative-prow
Bot
added
size/XL
|
Thanks @Itx-Psycho0! Rebased onto main and added a commit on top so the env-echo handler actually replaces each template's handler (the deployed function was still answering the stock "OK"). It's http-only, and it now passes across all runtimes, confirming /lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Itx-Psycho0, lkingland The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
Bot
added
the
approved
What This PR Does
This PR adds an end-to-end matrix test that verifies environment variables passed via the
-eflag during deployment actually work across all supported runtimes and builders.Background
Issue #3514 reported that environment variables passed with
func deploy -e KEY=VALUEweren't showing up in the deployed service. The user could see them working withfunc run -e, but when deploying to the cluster, the variables were missing from the container.After looking through the codebase, I noticed we have tests for
func config envs add, but there's no E2E test that actually verifies the-eflag works during deployment. We're testing the config management commands but not the actual deployment behavior with environment variables.@lkingland suggested adding an E2E matrix test that creates a function which reports back whether it received the environment variables, deploys it with
-eflags, invokes it, and checks if the variables made it through. This would run across all our supported combinations to catch any builder or runtime-specific issues.Implementation
I created
e2e/e2e_matrix_deploy_envs_test.gowith a new test functionTestMatrix_Deploy_Envs()that does the following for each runtime/builder/template combination:TEST_VARandANOTHER_VARfrom the environment and returns their values in the responsefunc deploy -e TEST_VAR=test_value -e ANOTHER_VAR=another_value --builder <builder>The test includes language-specific implementations for reading environment variables since each runtime does it differently:
os.Getenv()process.envos.getenv()env::var()@ConfigPropertyinjectionEnvironment.getProperty()Each implementation returns HTTP 500 if either variable is missing, and HTTP 200 with the values if both are present. This makes failures easy to detect.
Why This Approach
The test follows the same pattern as existing matrix tests like
TestMatrix_Deploy, using theforEachPermutationhelper to run across all valid combinations. It respects the same matrix exceptions and timeout configurations.By testing all combinations, we ensure the
-eflag works consistently whether you're using host builder with Go, pack builder with Python, or s2i builder with Node. If there's a bug in how a specific builder handles environment variables, this test will catch it.Expected Results
When this runs in CI:
-eflag works correctly and issue Environment variables passed via -e flag not applied on deploy #3514 can be closedThe test runs with
FUNC_E2E_MATRIX=true go test -v ./e2e -run TestMatrix_Deploy_Envsand will automatically run in CI when the matrix test environment variable is set.Testing
I've verified:
I haven't run the full matrix locally since it requires a Kubernetes cluster with all builders and runtime images, but the test follows the established patterns from other working matrix tests.
Fixes #3514