CNTRLPLANE-3037,OCPBUGS-78837: Introduce envtest for integration tests - #8016
CNTRLPLANE-3037,OCPBUGS-78837: Introduce envtest for integration tests#8016clebs wants to merge 2 commits into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@clebs: This pull request explicitly references no jira issue. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
Skipping CI for Draft Pull Request. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: clebs The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test all |
|
/test all |
|
@clebs: This pull request references CNTRLPLANE-3037 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
| // Start envtest with HyperShift CRDs | ||
| testEnv := &envtest.Environment{ | ||
| CRDDirectoryPaths: []string{crdDir}, | ||
| // Use TechPreviewNoUpgrade CRDs to include GCP validations |
There was a problem hiding this comment.
// Use TechPreviewNoUpgrade CRDs to include GCP validations
where is this enforced? the path contains also default CRD version. I assume last one would win and it's circumstantially TechPreview? which is fragile.
There was a problem hiding this comment.
we'll also want to install all the capi CRDs.
Basically what --install command with tech preview would apply.
There was a problem hiding this comment.
I took the simplest approach here as first try. Now looking closer at the e2e I see I missed the whole filtering logic on assets.
I will use that package the same way the original test does when installing so we can select the feature set to test. Maybe for now with an env var defaulting to TechPreviewNoUpgrade.
| cd $(TOOLS_DIR); $(GO) build -tags=tools -o $(SETUP_ENVTEST) sigs.k8s.io/controller-runtime/tools/setup-envtest | ||
|
|
||
| .PHONY: setup-envtest | ||
| setup-envtest: $(SETUP_ENVTEST) ## Setup envtest binaries (etcd, kube-apiserver) |
There was a problem hiding this comment.
I think we'll want to make this plumbing more flexible so we can test with different kas API versions
There was a problem hiding this comment.
There was a problem hiding this comment.
+1 Alberto
I don't care too much about running individual validation tests on older API servers, but I do think we need to at least take all CRDs and install them on the oldest OCP KAS that managed services are currently using (4.15 to my knowledge) and then as we start to enforce a skew policy and they upgrade to newer releases, we can bump that and hopefully get to a point where it is N-3 consistently going forward
There was a problem hiding this comment.
we need ability to test against oldest ocp (4.15) and oldest vanilla kube (to be confirmed).
There was a problem hiding this comment.
We currently have 4.17+ OCP KAS in the index, shouldn't be too hard to push 4.15 and 4.16
Upstream goes back to 1.28 (which is equiv to 4.15) so hopefully the minimum vanilla is at least that
|
/area api |
|
In openshift/api we made the choice to leverage YAML based testing for our integration tests. With the structured testing you currently have, you have no visibility into the marshalled content that is actually being sent to the API server. This can mask issues in your APIs (e.g. not having omitempty on a required field) so you may want to either us unstructured or switch to YAML as we do You'll also want to look at the possibility of testing ratcheting validations. Over time you may want to make validation tighter, and with that you need a way to install older, invalid schemas and check the ratcheting. It might be worth looking over our test suite for inspiration |
|
Let's adjust the PR to cover #8016 (comment) |
- Add an envtest setup goal on the Makefile - Add a new unit test using envtest to cover the same as the create cluster e2e test. Signed-off-by: Borja Clemente <bclement@redhat.com>
Introducing envtest requires adding new dependencies, which are being vendored in a separate commit to ease review. Signed-off-by: Borja Clemente <bclement@redhat.com>
|
|
||
| # Setup envtest for running tests that require a Kubernetes API server | ||
| # SETUP_ENVTEST_VER is the version of setup-envtest to use, matching the version in hack/tools/go.mod | ||
| KUBEBUILDER_ENVTEST_KUBERNETES_VERSION ?= 1.34.0 |
There was a problem hiding this comment.
4.22 is 1.35, pretty sure we have 1.35 binaries published that you can use
There was a problem hiding this comment.
I did take 1.34 because that is what hypershift is currently built with. If I should follow OCP k8s version instead I will look into this as part of the above effort to make the setup parametric.
| crdObjects := installassets.CustomResourceDefinitions( | ||
| func(path string, crd *apiextensionsv1.CustomResourceDefinition) bool { | ||
| if !strings.Contains(path, "hypershift-operator/") { | ||
| return false |
There was a problem hiding this comment.
this excludes all the API CRDs which we would want to include as well
|
/retitle CNTRLPLANE-3037,OCPBUGS-78837: Introduce envtest for integration tests |
|
@clebs: This pull request references CNTRLPLANE-3037 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.22.0" version, but no target version was set. This pull request references Jira Issue OCPBUGS-78837, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
The jira above was created to track this issue OCPBUGS-78837. Discussed on slack that this would be detected using this testEnv. |
|
superseded by #8034 |
|
PR needs rebase. 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. |
|
@clebs: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
/close An alternative PR merged #8089 |
|
@clebs: Closed 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. |
What this PR does / why we need it:
This PR introduces envtest and replicates the existing
TestOnCreateAPIUXe2e test (but not replacing it yet).Shifting part of the tests away from e2e into integration tests that do not require a full environment will help reduce the burden on our CI.
Checklist: