feat: use samples in the e2e (helm) #4028
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
needs-rebase
camilamacedo86
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
camilamacedo86
added
the
do-not-merge/hold
camilamacedo86
requested review from
asmacdo,
fabianvf and
jmrodri
and removed request for
bharathi-tenneti
camilamacedo86
changed the title
openshift-ci-robot
added
the
do-not-merge/work-in-progress
camilamacedo86
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
camilamacedo86
removed
the
do-not-merge/hold
| kbtestutils "sigs.k8s.io/kubebuilder/test/e2e/utils" | ||
|
|
||
| "github.com/operator-framework/operator-sdk/hack/generate/samples/internal/pkg" | ||
| "github.com/operator-framework/operator-sdk/internal/samples" |
There was a problem hiding this comment.
IMO we should avoid separate tooling-only code from code used in the SDK binary. Is there a reason this package move was necessary?
There was a problem hiding this comment.
We need to use the "github.com/operator-framework/operator-sdk/hack/generate/samples/internal/pkg" in the e2e tests. Then, it is not possible if it is in internal dir in the hack. So, the change here was only moved to "github.com/operator-framework/operator-sdk/internal/samples" instead of letting it exported. Just following your approach in another pr where was created the operator-sdk/internal/testutils/
| projectPath := strings.Split(current, "operator-sdk/")[0] | ||
| projectPath = strings.Replace(projectPath, "operator-sdk", "", 1) | ||
| helmChartPath := filepath.Join(projectPath, "operator-sdk/hack/generate/samples/internal/helm/testdata/memcached-0.0.1.tgz") | ||
| helmChartPath := filepath.Join(projectPath, "operator-sdk/hack/generate/samples/helm/testdata/memcached-0.0.1.tgz") |
There was a problem hiding this comment.
Somewhat tangential to this PR, but why are we going to all this trouble to find the project root? I would expect a hardcoded relative path here. If I happen to clone the repo in a directory path like /home/joe/operator-sdk/operator-sdk, this will fail.
There was a problem hiding this comment.
I agree that it is not the best solution. The problem is that in the tests the current path is not the same. I will try to improve that.
There was a problem hiding this comment.
I did a few improvements but it might be solved soon in the cleanups to perform changes and your suggestions
| By("mocking Helm Memcached Sample") | ||
| ctx, err := samples.NewSampleContextWithTestContext(&tc) | ||
| Expect(err).Should(Succeed()) | ||
| sample := helm.NewMemcachedHelm(&ctx) | ||
| sample.Run() |
There was a problem hiding this comment.
Why are we regenerating the sample in the e2e test? Let's just use the already generated sample in testdata.
There was a problem hiding this comment.
IMO: It would not be a good approach:
- We need to do changes in the e2e tests on top of the samples. For Ansible, for example, the e2e test mock needs to be Memcached-operator + molecule
- the e2e tests create its context (TestContext) which has other data which is not part of the SampleContext.
- it would increase the complexity to work with and contribute by the project since after doing a change and run the e2e tests we would need also to re-gen all samples again to came back it for its state. Otherwise, it will not pass in the test-sanity.
Anyway, I am very open to suggestion however, let's do it for all and then, after we make all work we can see what are the best halls for improvement. In this way, IHMO would be easier to see what is required and what is not. WDYT could we agree with?
There was a problem hiding this comment.
We need to do changes in the e2e tests on top of the samples. For Ansible, for example, the e2e test mock needs to be Memcached-operator + molecule
Taking a step back, theoretically the samples checked into testdata should be immediately testable and they should pass all tests, so I would expect very few necessary modifications (basically just modifications that would be required to run binaries and base images built from the PR, e.g. :dev image replacements)
the e2e tests create its context (TestContext) which has other data which is not part of the SampleContext.
I don't quite follow this. It seems like an implementation detail we can change to support running our normal e2e suite against existing files.
it would increase the complexity to work with and contribute by the project since after doing a change and run the e2e tests we would need also to re-gen all samples again to came back it for its state. Otherwise, it will not pass in the test-sanity.
We can avoid that by copying the entire sample into a temporary directory at the beginning of the test and then deleting that directory at the end of the test. The actual testdata directory does not need to be touched.
There was a problem hiding this comment.
Hi @joelanford,
I am not against new ideas and improvements at all. I am all in for that. However, I'd like to move forward here with what was proposed in operator-framework/enhancements#47 and do do not reduce/change scenarios/checks done in the e2e currently. See for example here that we still doing exactly the same validations and we do not change it. We only need to replace the Deployments per Pods since by using the helm-chart Memcached it will create Pods for the CR instances.
Note that, it is not part of the scope of the EP we reduce the scope of the E2E tests and if we do what you are suggesting we will by sure reduce the scope of the e2e tests done for Ansible and no longer do the same validations. The current e2e test made for ansible is a mock done based on memcached-operator samples + the molecule steps whcih is checking, for example, the blackList and finalizers feature that is not in its sample. See : https://github.com/operator-framework/operator-sdk/blob/master/test/e2e-ansible/e2e_ansible_suite_test.go#L106-L283 and https://github.com/operator-framework/operator-sdk/blob/master/testdata/ansible/memcached-operator/watches.yaml.
in this way, my idea here is
- we implement the EP [SDK] - Proposal for samples generation via GO and in the SDK repo enhancements#47
- then, after we will have the same e2e tests/checks/validations been done with the samples we will be in a better position to do a cleanup and/or improvements as you suggested.
Regards your idea to remove the SampleContext and use TestContext instead for all scenarios we have small changes that would need to be addressed in the upstream. It is totally fine. However, it is more one motivation to persuade my idea of we do it step by step.
Could we agree in re-check these options after all requirements are addressed and we have a better view over how we can improve it? Also, note that will be easier to check if something broke or not with the suggestions made after that and if has some feature/scenario that could no longer be part of the e2e tests.
|
Closing this in favor of #4134 which was done with @joelanford |
3 participants
Description of the change:
Motivation for the change:
operator-framework/enhancements#47