Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bug: cel tests are always run on a single version of Kubernetes #3584

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

liorokman
Copy link
Contributor

The cel test invocation was using the wrong verison of Kubernetes every time it was being run.

Here's what the Makefile did before the fix:

echo -e "\033[0;32m===========> Running go.test.cel ... \033[0m"
for ver in 1.27.1 1.28.0 1.29.0; do \
  		echo "Run CEL Validation on k8s $ver"; \
        go clean -testcache; \
        KUBEBUILDER_ASSETS="/home/user/.local/share/kubebuilder-envtest/k8s/1.30.0-linux-amd64" \
         go test ./test/cel-validation --tags celvalidation -race; \
    done

Notice that the KUBEBUILDER_ASSETS is already hardcoded to version 1.30. This is because the $(shell) expansion used is evaluated only once before make runs the for loop.

Here's what it looks like after the fix:

echo -e "\033[0;32m===========> Running go.test.cel ... \033[0m"
for ver in 1.27.1 1.28.0 1.29.0; do \
  		echo "Run CEL Validation on k8s $ver"; \
        go clean -testcache; \
        KUBEBUILDER_ASSETS="$(tools/bin/setup-envtest use $ver -p path)" \
         go test ./test/cel-validation --tags celvalidation -race; \
    done

Notice that this time setup-envtest is called with the correct version every time.

@liorokman liorokman requested a review from a team as a code owner June 11, 2024 07:26
Copy link

codecov bot commented Jun 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.14%. Comparing base (84921e5) to head (9693b72).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3584      +/-   ##
==========================================
- Coverage   68.16%   68.14%   -0.02%     
==========================================
  Files         168      168              
  Lines       20408    20408              
==========================================
- Hits        13912    13908       -4     
- Misses       5494     5497       +3     
- Partials     1002     1003       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zirain
Copy link
Member

zirain commented Jun 11, 2024

what your machine? linux or mac?

@liorokman
Copy link
Contributor Author

It's a Linux machine. Why is this relevant?

@zirain
Copy link
Member

zirain commented Jun 11, 2024

It's a Linux machine. Why is this relevant?

it works as expected on my m1 mac

make go.test.cel
===========> Running generate-gwapi-manifests ... 
curl -sLo /Users/zirain/Codes/servicemesh/gateway/bin/gatewayapi-crds.yaml https://github.com/kubernetes-sigs/gateway-api/releases/download/v1.1.0/experimental-install.yaml
mv /Users/zirain/Codes/servicemesh/gateway/bin/gatewayapi-crds.yaml charts/gateway-helm/crds/gatewayapi-crds.yaml
===========> Running manifests ... 
tools/bin/controller-gen crd:allowDangerousTypes=true paths="./..." output:crd:artifacts:config=charts/gateway-helm/crds/generated
===========> Running go.test.cel ... 
Run CEL Validation on k8s 1.27.1
ok      github.com/envoyproxy/gateway/test/cel-validation       9.314s
Run CEL Validation on k8s 1.28.0
ok      github.com/envoyproxy/gateway/test/cel-validation       9.124s
Run CEL Validation on k8s 1.29.0
ok      github.com/envoyproxy/gateway/test/cel-validation       8.431s

@liorokman
Copy link
Contributor Author

liorokman commented Jun 11, 2024

It prints the right value in the echo statement, but it's using the wrong Kubernetes version to run the test.

Can you try to run make go.test.cel -n in your environment? This will show you what is really being run without actually running it.

Check the value of the KUBEBUILDER_ASSETS in that output.

@zirain
Copy link
Member

zirain commented Jun 11, 2024

Thanks for cathing this.

@zirain zirain merged commit 561b979 into envoyproxy:main Jun 11, 2024
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants