[run-bundle] Test example scenarios for run bundle by varshaprasad96 · Pull Request #3844 · operator-framework/operator-sdk

varshaprasad96 · 2020-09-02T22:28:33Z

Description of the change:
This PR adds additional olm integration tests to test the run bundle
command for various example UX scenarios.

Motivation for the change:
Follow up of #3831

c/c: @jmrodri

Checklist

If the pull request includes user-facing changes, extra documentation is required:

Add a new changelog fragment in changelog/fragments (see changelog/fragments/00-template.yaml)
Add or update relevant sections of the docs website in website/content/en/docs

camilamacedo86 · 2020-09-02T22:43:10Z

+	)
+	BeforeEach(func() {
+		By("creating a new context")
+		// For this test suite, the bundle image name is equal to tc.ImageName


I think the comment is. not. required since the TestContext has the ImageName attribute to store that. So, shows that it is implicit.

I had written it explicitly to specify that tc.ImageName is not operator image but bundle image instead.

camilamacedo86 · 2020-09-02T22:58:35Z

+		i := bundle.NewInstall(cfg)
+		i.BundleImage = tc.ImageName


You are running the make bundle for he image. what the NewInstall does? Why it is required?

The bundle image is an argument to the run bundle command. When the command is passed, the Install struct is populated. It is referenced here: https://github.com/operator-framework/operator-sdk/blob/master/internal/olm/operator/bundle/install.go#L42

why not we do not use operator-sdk run bundle here? IMO : It is an e2e test so we should use the CLI commands otherwise we could cover these scenarios with unit tests.

camilamacedo86 · 2020-09-02T22:58:48Z

+		cfg := &operator.Configuration{KubeconfigPath: kubeconfigPath}
+		err := cfg.Load()
+		Expect(err).NotTo(HaveOccurred())


Why it is rerquired?

These are a part of run bundle arguments. The kubecfg (specifying the namespace if needed), and the bundle image is passed to the run bundle.
For reference: It is similar to how testing is done in packagemanifests-> https://github.com/operator-framework/operator-sdk/blob/master/test/integration/operator_olm_test.go#L106

camilamacedo86 · 2020-09-02T22:59:25Z

+		err = doInstall(i)
+		Expect(err).NotTo(HaveOccurred())


What the doinstall does?

doinstall is the wrapper, which in turn uses the OperatorInstaller and installs the csv using olm.
For reference, deinstall implementation is here, which in turn calls the bundle's Run implementation.

Same above. we need to use the CLI command instead.

camilamacedo86

IMO we should NOT create the test in the "integration" dir.

We need to check the command to run the bundle in the e2e tests which would be called at the same that the users will use it. See that in the e2e test the bundle is generated all in the same way for Ansible/Helm/GO. So we need to run operator-sdk run bundle for the projects instead of it. See https://github.com/operator-framework/operator-sdk/blob/master/test/e2e-helm/e2e_helm_olm_test.go#L51-L63

camilamacedo86 · 2020-09-02T23:13:26Z

+	uninstallConfig.DeleteAll = true
+	uninstallConfig.DeleteOperatorGroupNames = []string{tc.Group}
+	uninstallConfig.Package = defaultOperatorName
+	uninstallConfig.Logf = logrus.Infof


what should be uninstalled here? Is the bundle?

Yes, it is the bundle. As you said, after creating a new test context for the entire suite, before each test we uninstall the previous bundle (csv, crd and other created objects).

Will not the tool provide a command to uninstall?

camilamacedo86 · 2020-09-02T23:15:40Z

+func TestRunBundle(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Operator SDK run bundle test suite")
+}


in order to follow up a standard each dir should have 1 suite test and the tests that will be called by it. IMO we need re-check if is really required to keep this dir.

As discussed with @jmrodri and @estroz, this PR was structured to follow the run packagemanifests format. We add the generic run bundle test to operator e2e test suite which is done here: #3831 and the tests to check various scenarios in the olm integration suite. Similar to how package manifests does: test/integration/operator_olm_test.go.
I am not sure if these tests would be run when the user calls run bundle, because we are using a pre-generated bundle specific for this tests.

OK. See that :

we cover the features with unit test

we have e2e tests for each type of project where we will test the run bundle with ALL Namespaces.

So, this e2e test would just make sense if we would like to cover all possible scenarios. Is this the goal? If yes, OK. However, note that test/integration/operator_olm_test.go is old and is NOT follow the conventions and standards defined so far.

So, if we will add tests to cover all options of the run bundle command with e2e then:

we should use the libs defined and then follow the standards and conventions defined by them and applied in the ansible/helm tests already. (we have pr to cleanup go implementation waiting for reviews feat: cleanup go e2e tests and perform improvements )

the integration tests need to be in test/e2e-<name>. I suggest we have a dir test/e2e-olm. (we can leave for this integration dir, however, I'd like to see the new tests following the same standard and convections)

The directory requires an e2e_<name>_suite_test which will have the suite and then we have the files tests. Note that it is the standard of the libs adopted.

NOTE: You can create the dir and run the command $ ginkgo bootstrap ** and then, you will check the files in this way.

we should not re-create the project for each test. the project can be created in the before of the suite and used by all test and the removed on the after of the suite instead. See Ansible/Helm suite tests.

the go e2e test should have the suffix _test which has special meaning for the Go compiler

c/c @jmrodri @estroz

This PR adds additional olm integration tests to test the run bundle command for various example UX scenarios.

estroz · 2020-09-04T17:16:28Z

+func TestRunBundle(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecs(t, "Operator SDK run bundle test suite")
+}


All integration tests in this package should be converted to ginkgo/gomega tests eventually like this integration test is, so the suite runner should be in a separate file and look like so:

Suggested change

func TestRunBundle(t *testing.T) {

RegisterFailHandler(Fail)

RunSpecs(t, "Operator SDK run bundle test suite")

}

func TestIntegration(t *testing.T) {

RegisterFailHandler(Fail)

RunSpecs(t, "Integration Suite")

}

IMO we need to create the dir /test/e2e-olm/ in order to follow the project standard. The tests here are old and are not following the new definitions. Then, we should run ginkgo bootstrap. It will generated the files with the names following the lib convention. See: https://onsi.github.io/ginkgo/

estroz · 2020-09-04T17:21:42Z

+			Skip("Run bundle not yet implemented")
+		}
+
+		cfg := &operator.Configuration{KubeconfigPath: kubeconfigPath}


This may not be testing OwnNamespace since the default namespace used might not be the one created for this test (tc.Kubectl.Namespace).

estroz · 2020-09-04T17:22:29Z

+			Skip("Run bundle not yet implemented")
+		}
+
+		cfg := &operator.Configuration{KubeconfigPath: kubeconfigPath, Namespace: tc.Kubectl.Namespace}


If tc.Kubectl.Namespace == i.InstallMode.TargetNamespaces then you're actually using the OwnNamespace install mode.

It is an e2e test and we are creating the project. So, we need to test it as users would be performing this action. Have we a flag to define the run bundle is single namespace or it users need to update the manifest generated?

estroz · 2020-09-04T17:23:21Z

+		uninstall(kubeconfigPath, tc)
+	})
+
+	It("Run Bundle - watching the specified namesapce", func() {


What do you mean by "watching the specified namespace"? This case is testing SingleNamespace. I suggest you get rid of the above It() and just use this one.

estroz · 2020-09-04T17:24:12Z

+})
+
+var _ = Describe("run bundle", func() {
+	It("Run Bundle Basic", func() {


Suggested change

It("Run Bundle Basic", func() {

It("AllNamespaces (default)", func() {

camilamacedo86 · 2020-09-04T18:09:13Z

+
+// Modified from https://github.com/kubernetes-sigs/kubebuilder/tree/39224f0/test/e2e/v3
+
+package e2e


// should be the name of the dir with _test
Assuming that it will be in /tests/e2e_olm as suggested:

Suggested change

package e2e

package e2e_olm_test

camilamacedo86 · 2020-09-04T18:11:01Z

+	By("implementing the API")
+	Expect(kbtestutils.InsertCode(
+		filepath.Join(tc.Dir, "api", tc.Version, fmt.Sprintf("%s_types.go", strings.ToLower(tc.Kind))),
+		fmt.Sprintf(`type %sSpec struct {
+`, tc.Kind),
+		`	// +optional
+	Count int `+"`"+`json:"count,omitempty"`+"`"+`
+`)).Should(Succeed())


It shows not required for the test at all. Why we need to add an value in the types? Could we not use the foo?

camilamacedo86 · 2020-09-04T18:13:47Z

+	corev1 "k8s.io/api/core/v1"
+	wait "k8s.io/apimachinery/pkg/util/wait"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	kbtestutils "sigs.k8s.io/kubebuilder/test/e2e/utils"


the imports need to be sorted.

camilamacedo86 · 2020-09-04T18:21:05Z

+	By("push the bundle to a remote repository")
+	err = tc.Make("docker-push", "IMG="+tc.ImageName)
+	Expect(err).NotTo(HaveOccurred())


Suggested change

By("push the bundle to a remote repository")

err = tc.Make("docker-push", "IMG="+tc.ImageName)

Expect(err).NotTo(HaveOccurred())

we should not use the docker-push here. The tests are executed using Kind. We build the images and then we load them. See:

https://github.com/operator-framework/operator-sdk/blob/master/test/e2e-ansible/e2e_ansible_olm_test.go#L62-L68

varshaprasad96 · 2020-09-30T02:54:08Z

Closing this PR as this would be tested in unit tests.

openshift-ci-robot requested review from camilamacedo86 and estroz September 2, 2020 22:28

varshaprasad96 requested a review from jmrodri September 2, 2020 22:28

camilamacedo86 reviewed Sep 2, 2020

View reviewed changes

Comment thread test/integration/operator_run_bundle_test.go Outdated

camilamacedo86 reviewed Sep 2, 2020

View reviewed changes

camilamacedo86 requested changes Sep 2, 2020

View reviewed changes

camilamacedo86 reviewed Sep 2, 2020

View reviewed changes

[run-bundle] Test example scenarios for run bundle

d0e3f33

This PR adds additional olm integration tests to test the run bundle command for various example UX scenarios.

varshaprasad96 force-pushed the run-bundle/test-scenario branch from 727dc50 to d0e3f33 Compare September 3, 2020 18:12

estroz reviewed Sep 4, 2020

View reviewed changes

camilamacedo86 reviewed Sep 4, 2020

View reviewed changes

jmrodri mentioned this pull request Sep 8, 2020

Implement run bundle subcommand to improve OLM integration #3626

Closed

14 tasks

varshaprasad96 closed this Sep 30, 2020

	It("Run Bundle Basic", func() {
	It("AllNamespaces (default)", func() {


		// Modified from https://github.com/kubernetes-sigs/kubebuilder/tree/39224f0/test/e2e/v3

		package e2e

	By("push the bundle to a remote repository")
	err = tc.Make("docker-push", "IMG="+tc.ImageName)
	Expect(err).NotTo(HaveOccurred())

Conversation

varshaprasad96 commented Sep 2, 2020

Uh oh!

camilamacedo86 Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

camilamacedo86 Sep 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varshaprasad96 Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varshaprasad96 Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camilamacedo86 Sep 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

varshaprasad96 commented Sep 30, 2020

Uh oh!

Reviewers

Assignees

camilamacedo86 Sep 2, 2020 •

edited

Loading

camilamacedo86 Sep 2, 2020 •

edited

Loading

camilamacedo86 left a comment •

edited

Loading

varshaprasad96 Sep 3, 2020 •

edited

Loading

varshaprasad96 Sep 3, 2020 •

edited

Loading

camilamacedo86 Sep 4, 2020 •

edited

Loading

camilamacedo86 Sep 4, 2020 •

edited

Loading

camilamacedo86 Sep 4, 2020 •

edited

Loading