🌱 Add e2e tests for sample external plugin #3419

Eileen-Yu · 2023-05-18T22:56:02Z

Description:

Add e2e tests for sample external plugin

Motivation:

Validate the functionality and the behavior of the sample external plugin.

k8s-ci-robot · 2023-05-18T22:56:11Z

Hi @Eileen-Yu. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

test/e2e/externalplugin/e2e_suite_test.go

everettraven · 2023-05-19T13:20:02Z

/ok-to-test

everettraven

Thanks @Eileen-Yu for putting this together! For the most part it looks good - great work!

There are just a few nits and a couple more things that I think need to be done to ensure we are including these tests in CI:

test/e2e/externalplugin/generate_test.go

test/e2e/externalplugin/e2e_suite_test.go

test/e2e/externalplugin/generate_test.go

rashmigottipati · 2023-05-24T12:53:47Z

/retest

camilamacedo86 · 2023-05-24T13:03:19Z

Makefile

 	## To keep the same kind cluster between test runs, use `SKIP_KIND_CLEANUP=1 make test-e2e-local`
 	./test/e2e/local.sh

 .PHONY: test-e2e-ci
-test-e2e-ci: ## Run the end-to-end tests (used in the CI)`
+test-e2e-ci: build-external-plugin ## Run the end-to-end tests (used in the CI)`


Revert this change you added it in the prow already

I think this is still needed. IIUC, just the execution of the go tests were added in the Prow configuration

The prow setup is done in this one: https://github.com/kubernetes-sigs/kubebuilder/blob/master/test/e2e/setup.sh#L56-L67

We do not call any of our tests in this way, so why we would need to have this new target and call in the test-e2e-ci?

rashmigottipati · 2023-05-24T13:04:41Z

test/e2e/build-sample-external-plugin.sh

+if [ $? -eq 0 ]; then
+    echo "External plugin is built at: $EXTERNAL_PLUGIN_DESTINATION"
+else
+    echo "FAIL to build external plugin"


It would be nice to echo the exact error message as well, with which init command has failed.

IMO, ideally, we need to keep all in Go.
Why? It makes it easier for us to keep the project maintained.

See that using an IDE, we can trigger the tests directly, which means the sh would not be executed.

Also, it makes it more intuitive since all the required steps to do the tests are placed together.

By leasing, keeping it in Go with the test help us to troubleshot since we can add breakpoints to verify each step

@camilamacedo86 I don't disagree that it is ideal but I think we already have precedence for building the binaries for testing purposes in shell scripts. I think for now we should follow the pre-existing pattern and we can make an issue to convert the e2e shell scripts to Go

I like @everettraven idea to keep the pattern consistent in this PR.

Other than that, it feels like this is something suitable in the scope of the external plugin project itself.
Such as a make target in the sample external plugin folder.

If we want to keep the pattern consistent accross the project then we should not add what is required to be build in the tests in shell. Note that today we can run the e2e tests locally for dev purposes without any shell. We just run the suite in go. So that will broke the current behaviour

See: #3419 (comment)

I agree it is a good practice to follow the current pattern, which lets Ginkgo do all e2e related tasks, instead of having an additional script. By doing this we wish to keep consistency and benefit from Ginkgo's feature.

The current script may be a reference to put in the docs to provide users info about how to install their external plugin.

camilamacedo86 · 2023-05-24T13:09:39Z

test/e2e/externalplugin/generate_test.go

+func GenerateProject(kbc *utils.TestContext) {
+	var err error
+
+	By("initializing a project")


See that before init you need to ensure that the plugin was built.
It is not better build the plugin here instead of create the sh?
Could we not move the code to here and to do that in go so that is easier to keep maintained as well?

See that you can run it locally either if you move all to here by just trigging the tests from your IDE

I don't disagree that it might be easier to maintain, but I think there is a lot of precedence to build stuff in a shell script for the e2e tests. IMO we shouldn't try to reinvent the wheel by adding this to the go code and follow the existing pattern for building and configuring e2e tests with shell scripts.

Hi @everettraven,

but I think there is a lot of precedence to build stuff in a shell script for the e2e tests.

We currently do not utilize any shell scripting to accomplish the tasks required for our end-to-end (e2e) tests. It's important to note that these tests can be executed locally as long as you have a functional Kubernetes cluster. However, after this PR is merged, this functionality will likely be disrupted and might no longer be possible.

To elaborate, introducing shell scripting for e2e tests as we are doing here will break the development environment. This is contrary to our current setup. The shells scripts in place are Optional to setup the k8s locally either and btw I never used them or to call the test on prow.

Nonetheless, if @Eileen-Yu is able to successfully integrate shell scripting without compromising the functionality (despite my concerns that it will negatively affect the ability to run e2e tests in a development environment), I'm open to having this PR merged and making necessary changes in a follow-up. Please note, however, that it appears these tests are currently failing due to the lack of the necessary steps within the e2e test environment. Therefore, when the init command is called, it results in a failure. See the following for more details:"

s: "kubebuilder init --plugins sampleexternalplugin/v1 --domain sample.domain.com failed with error: (exit status 1) Error: no plugin could be resolved with key \"sampleexternalplugin/v1\"\nUsage:\n kubebuilder [flags]\n\nExamples:\nThe first step is to initialize your project:\n kubebuilder init [--plugins=<PLUGIN KEYS> [--project-version=<PROJECT VERSION>]]\n\n<PLUGIN KEYS> is a comma-separated list of plugin keys from the following table\nand <PROJECT VERSION> a supported project version for these plugins.\n\n Plugin keys | Supported project versions\n-----------------------------------------+----------------------------\n base.go.kubebuilder.io/v3 | 3\n base.go.kubebuilder.io/v4 | 3\n declarative.go.kubebuilder.io/v1 | 2, 3\n deploy-image.go.kubebuilder.io/v1-alpha | 3\n go.kubebuilder.io/v2 | 2, 3\n go.kubebuilder.io/v3 | 3\n go.kubebuilder.io/v4 | 3\n grafana.kubebuilder.io/v1-alpha | 3\n kustomize.common.kubebuilder.io/v1 | 3\n kustomize.common.kubebuilder.io/v2 | 3\n\nFor more specific help for the init command of a certain plugins and project version\nconfiguration please run:\n kubebuilder init --help --plugins=<PLUGIN KEYS> [--project-version=<PROJECT VERSION>]\n\nDefault plugin keys: \"go.kubebuilder.io/v4\"\nDefault project version: \"3\"\n\n\nFlags:\n -h, --help help for kubebuilder\n --plugins strings plugin keys to be used for this subcommand execution\n --project-version string project version (default \"3\")\n\n2023/05/24 13:03:58 no plugin could be resolved with key \"sampleexternalplugin/v1\"\n", } occurred

Hey @camilamacedo86 , I'm trying to follow your idea:

Since test/e2e/utils provides context to bring rich prerequisites, including k8s cluster, kubectl, kubebuilder.
That is how e2e tests get performed for internal plugins.

So, it is not necessary/consistent to deal with prerequisites on a different pattern, such as running a new script.

Is that something you mentioned of and such external-plugin should NOT break?

We currently do not utilize any shell scripting to accomplish the tasks required for our end-to-end (e2e) tests.

Here is my opinion:
We have a well-structured directory under test/e2e:

those single files such as local.sh, setup.sh are used to provide the general environment for e2e testing

plugin related stuff are put into separate files such as v4, deployimage

To maintain that way, when we create a new e2e test suite for external-plugin, it is not necessary/recommended to touch those public files in 1.

Then, if building and configuring external-plugin is needed, we can follow what we did to other plugins:

At the very beginning in the ginkgo context, use go code to build and configure external-plugin

At the end of the ginkgo context, delete the temporary asset for external-plugin

everettraven · 2023-05-24T13:09:03Z

test/e2e/build-sample-external-plugin.sh

+SOURCE_DIR="./docs/book/src/simple-external-plugin-tutorial/testdata/sampleexternalplugin/v1"
+cd $SOURCE_DIR && go build -o $PLUGIN_NAME && mv $PLUGIN_NAME "$EXTERNAL_PLUGIN_PATH"
+
+kubebuilder init --plugins="${PLUGIN_NAME}/${PLUGIN_VERSION}" --help | grep --quiet "$PLUGIN_NAME"


Nice touch, but this introduces a dependency on the kubebuilder binary being in your PATH when you run this script. Ideally the build script will not use something in the PATH but rather a location we can control during the build. There are a couple options here:

We remove this line and don't test that kubebuilder can initialize with this plugin as part of building the plugin

We build a local version of the kubebuilder binary from source to use for testing this as part of this script

Looks like we already have a shell function for building kubebuilder here:

kubebuilder/test/common.sh

Line 104 in 804fed4

function build_kb {

everettraven · 2023-05-24T13:11:44Z

Makefile

 	## To keep the same kind cluster between test runs, use `SKIP_KIND_CLEANUP=1 make test-e2e-local`
 	./test/e2e/local.sh

 .PHONY: test-e2e-ci
-test-e2e-ci: ## Run the end-to-end tests (used in the CI)`
+test-e2e-ci: build-external-plugin ## Run the end-to-end tests (used in the CI)`


I think this is still needed. IIUC, just the execution of the go tests were added in the Prow configuration

everettraven · 2023-05-24T13:11:56Z

test/e2e/build-sample-external-plugin.sh

+if [ $? -eq 0 ]; then
+    echo "External plugin is built at: $EXTERNAL_PLUGIN_DESTINATION"
+else
+    echo "FAIL to build external plugin"


everettraven · 2023-05-24T13:21:33Z

test/e2e/build-sample-external-plugin.sh

+# TODO: Dynamically set exteranl plugin destination based on OS platform
+EXTERNAL_PLUGIN_DESTINATION_PREFIX="${HOME}/Library/Application Support/kubebuilder/plugins"


This is making me wonder if it would be worth implementing the functionality to configure where kubebuilder should search for external plugins sooner rather than later. I'm not a huge fan of having e2e tests making modifications to a HOME directory.

I completely agree with your assessment.

Modifying the HOME directory during e2e testing is indeed not an ideal scenario, and it may lead to unforeseen issues or conflicts.

I think allowing users to configure the path for searching external plugins would significantly increase the flexibility and control over the system.

Wondering if it's possible to read a configuration file or an environment variable provided by the user, to specify the desired path for the plugin search? Hope in this way we could avoid making assumptions about the user's directory structure or enforcing unnecessary constraints.

One approach maybe:
Introduce an env var KB_PLUGIN_PATH when executing the kb command to tell the path of the external plugin.

KB_PLUGIN_PATH="/opt/app/kubebuilder/plugin" \ kubebuilder create api --plugins=myexternalplugin/v1

If such env does not exist, fall back to the default (currently used) path.

everettraven · 2023-05-24T13:25:34Z

test/e2e/externalplugin/generate_test.go

+func GenerateProject(kbc *utils.TestContext) {
+	var err error
+
+	By("initializing a project")


I don't disagree that it might be easier to maintain, but I think there is a lot of precedence to build stuff in a shell script for the e2e tests. IMO we shouldn't try to reinvent the wheel by adding this to the go code and follow the existing pattern for building and configuring e2e tests with shell scripts.

everettraven · 2023-05-24T13:30:05Z

test/e2e/build-sample-external-plugin.sh

I think the shell script approach is good, however I'm not sure this needs its own shell script. Taking a look at the overall test/ directory, there is a common.sh file that contains a bunch of shell functions to build and configure things for e2e tests and then are used like:

kubebuilder/test/e2e/setup.sh

Lines 17 to 19 in 804fed4

build_kb

fetch_tools

install_kind

My recommendation would be that we try to follow the existing pattern as much as possible

For clarification, my recommendation is that we:

Make this shell script a new function in the common.sh file

call this function in the test/e2e/setup.sh script similar to the snippet I shared above

camilamacedo86 · 2023-05-26T15:41:19Z

test/e2e/build-sample-external-plugin.sh

+set -e
+
+# TODO: Dynamically set exteranl plugin destination based on OS platform
+EXTERNAL_PLUGIN_DESTINATION_PREFIX="${HOME}/Library/Application Support/kubebuilder/plugins"


@everettraven @rashmigottipati Will this path exist in the prow ?
I think we might need to put at in test/e2e/externalplugin/bin

Co-authored-by: Bryce Palmer <everettraven@gmail.com>

Kavinjsir

lgtm

nits: better to set the external plugin path flexibly.

everettraven

This looks great! Awesome work @Eileen-Yu !

/lgtm

rashmigottipati

Great work @Eileen-Yu,
/lgtm

camilamacedo86

Great work 🥇
We can improve it in follow ups.

That is a really amazing job

k8s-ci-robot · 2023-05-31T15:21:11Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Eileen-Yu, everettraven, Kavinjsir, rashmigottipati

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [camilamacedo86]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 18, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 18, 2023

k8s-ci-robot requested review from joelanford and varshaprasad96 May 18, 2023 22:56

Eileen-Yu force-pushed the external-plugin-e2e branch 2 times, most recently from 1e971df to 7c65085 Compare May 19, 2023 00:14

camilamacedo86 reviewed May 19, 2023

View reviewed changes

test/e2e/externalplugin/e2e_suite_test.go Show resolved Hide resolved

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 19, 2023

everettraven suggested changes May 19, 2023

View reviewed changes

Kavinjsir suggested changes May 19, 2023

View reviewed changes

test/e2e/externalplugin/generate_test.go Show resolved Hide resolved

test/e2e/externalplugin/generate_test.go Outdated Show resolved Hide resolved

Eileen-Yu force-pushed the external-plugin-e2e branch from 5a28069 to 81980a5 Compare May 24, 2023 01:19

camilamacedo86 reviewed May 24, 2023

View reviewed changes

rashmigottipati reviewed May 24, 2023

View reviewed changes

camilamacedo86 reviewed May 24, 2023

View reviewed changes

everettraven reviewed May 24, 2023

View reviewed changes

camilamacedo86 reviewed May 26, 2023

View reviewed changes

test: add e2e tests for sample external plugin

597d2d6

Co-authored-by: Bryce Palmer <everettraven@gmail.com>

Eileen-Yu force-pushed the external-plugin-e2e branch from 81980a5 to 78b7b50 Compare May 30, 2023 20:25

ci: add a script to install sample external plugin

926dd60

Eileen-Yu force-pushed the external-plugin-e2e branch from 78b7b50 to 926dd60 Compare May 30, 2023 21:38

Kavinjsir approved these changes May 31, 2023

View reviewed changes

everettraven approved these changes May 31, 2023

View reviewed changes

k8s-ci-robot assigned everettraven May 31, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2023

rashmigottipati approved these changes May 31, 2023

View reviewed changes

k8s-ci-robot assigned rashmigottipati May 31, 2023

camilamacedo86 approved these changes May 31, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 31, 2023

k8s-ci-robot merged commit 40ec611 into kubernetes-sigs:master May 31, 2023
15 checks passed

Eileen-Yu mentioned this pull request Jun 7, 2023

REQUEST: New membership for Eileen-Yu kubernetes/org#4270

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🌱 Add e2e tests for sample external plugin #3419

🌱 Add e2e tests for sample external plugin #3419

Eileen-Yu commented May 18, 2023

k8s-ci-robot commented May 18, 2023

everettraven commented May 19, 2023

everettraven left a comment

rashmigottipati commented May 24, 2023

camilamacedo86 May 24, 2023

everettraven May 24, 2023

camilamacedo86 May 24, 2023

rashmigottipati May 24, 2023

everettraven May 24, 2023

camilamacedo86 May 24, 2023

everettraven May 24, 2023

Kavinjsir May 24, 2023

camilamacedo86 May 24, 2023

Eileen-Yu May 24, 2023 •

edited

Loading

camilamacedo86 May 24, 2023

everettraven May 24, 2023

camilamacedo86 May 24, 2023 •

edited

Loading

Kavinjsir May 24, 2023 •

edited

Loading

everettraven May 24, 2023

everettraven May 24, 2023

everettraven May 24, 2023

everettraven May 24, 2023

everettraven May 24, 2023

Eileen-Yu May 24, 2023

Kavinjsir May 26, 2023

everettraven May 24, 2023

everettraven May 24, 2023

everettraven May 24, 2023

camilamacedo86 May 26, 2023 •

edited

Loading

Kavinjsir left a comment

everettraven left a comment

rashmigottipati left a comment

camilamacedo86 left a comment

k8s-ci-robot commented May 31, 2023

		# TODO: Dynamically set exteranl plugin destination based on OS platform
		EXTERNAL_PLUGIN_DESTINATION_PREFIX="${HOME}/Library/Application Support/kubebuilder/plugins"

🌱 Add e2e tests for sample external plugin #3419

🌱 Add e2e tests for sample external plugin #3419

Conversation

Eileen-Yu commented May 18, 2023

Description:

Motivation:

k8s-ci-robot commented May 18, 2023

everettraven commented May 19, 2023

everettraven left a comment

Choose a reason for hiding this comment

rashmigottipati commented May 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eileen-Yu May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Kavinjsir May 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

camilamacedo86 May 26, 2023 • edited Loading

Choose a reason for hiding this comment

Kavinjsir left a comment

Choose a reason for hiding this comment

everettraven left a comment

Choose a reason for hiding this comment

rashmigottipati left a comment

Choose a reason for hiding this comment

camilamacedo86 left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented May 31, 2023

Eileen-Yu May 24, 2023 •

edited

Loading

camilamacedo86 May 24, 2023 •

edited

Loading

Kavinjsir May 24, 2023 •

edited

Loading

camilamacedo86 May 26, 2023 •

edited

Loading