✨ Allow passing the assets path bin via the env config #1214

camilamacedo86 · 2020-10-10T14:05:51Z

Description

Allow passing the assets path bin via the env config

        By("bootstrapping test environment")
	testEnv = &envtest.Environment{
		CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")},
                BinaryAssetsDirectory: "filepath.Join("..", "bin")" // See here
	}

The priority here is:

the specific envvars for each binary: TEST_ASSET_KUBE_APISERVER, TEST_ASSET_ETCD, TEST_ASSET_KUBECTL
the path for all env var: KUBEBUILDER_ASSETS
the value set via the structure in this case Environment{}
the default value which is currently /usr/local/kubebuilder/bin

Motivation

Allow passing the bin path to simplify the required implementation to config the EnvTest when the bin is not in the default path. Currently, if a developer would like to try to get the bins from a specific dir if they exist would be required an implementation such as:

var _ = BeforeSuite(func(done Done) {
	logf.SetLogger(zap.LoggerTo(GinkgoWriter, true))

	By("bootstrapping test environment")
	testEnv = &envtest.Environment{
		CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")},
	}

	By("configuring env test binaries")
	configEnvTestBinaries()

	var err error
	cfg, err = testEnv.Start()
	Expect(err).ToNot(HaveOccurred())
	Expect(cfg).ToNot(BeNil())

	err = crewv1.AddToScheme(scheme.Scheme)
	Expect(err).NotTo(HaveOccurred())

	// +kubebuilder:scaffold:scheme

	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
	Expect(err).ToNot(HaveOccurred())
	Expect(k8sClient).ToNot(BeNil())

	close(done)
}, 60)

var _ = AfterSuite(func() {
	By("tearing down the test environment")
	err := testEnv.Stop()
	Expect(err).ToNot(HaveOccurred())
})

// configEnvTestBinaries will check if the required binaries are in the bin directory
// of the project and configure to use them.
func configEnvTestBinaries() {
	binPath := filepath.Join("..", "..", "bin")
	if hasEnvTestBinaries(binPath) {
		os.Setenv("KUBEBUILDER_ASSETS", binPath)
	}
}

// hasEnvTestBinaries will return true when the kubebuilder tools binaries are found
// in the path informed
func hasEnvTestBinaries(path string) bool {
	if _, err := os.Stat(path); os.IsNotExist(err) {
		return false
	}
	if _, err := os.Stat(filepath.Join(path, "etcd")); os.IsNotExist(err) {
		return false
	}
	if _, err := os.Stat(filepath.Join(path, "kube-apiserver")); os.IsNotExist(err) {
		return false
	}
	if _, err := os.Stat(filepath.Join(path, "kubectl")); os.IsNotExist(err) {
		return false
	}
	return true
}

With this change we can just:

var _ = BeforeSuite(func(done Done) {
	logf.SetLogger(zap.LoggerTo(GinkgoWriter, true))

	By("bootstrapping test environment")
	testEnv = &envtest.Environment{
		CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")},
                BinaryAssetsDirectory: "filepath.Join("..", "bin")" // See here
	}

	var err error
	cfg, err = testEnv.Start()
	Expect(err).ToNot(HaveOccurred())
	Expect(cfg).ToNot(BeNil())

	err = crewv1.AddToScheme(scheme.Scheme)
	Expect(err).NotTo(HaveOccurred())

	// +kubebuilder:scaffold:scheme

	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
	Expect(err).ToNot(HaveOccurred())
	Expect(k8sClient).ToNot(BeNil())

	close(done)
}, 60)

var _ = AfterSuite(func() {
	By("tearing down the test environment")
	err := testEnv.Stop()
	Expect(err).ToNot(HaveOccurred())
})

camilamacedo86 · 2020-10-12T15:12:24Z

/test pull-controller-runtime-test-master

camilamacedo86 · 2020-10-17T13:33:49Z

/test pull-controller-runtime-test-master

DirectXMan12

EDIT: Overall, I think the priority should change a bit -- environment variables are intended as overrides, so I think they should be on top. Otherwise, if I store things elsewhere on my system, or want to test with a different version, I can't

DirectXMan12 · 2020-11-19T00:00:41Z

pkg/envtest/server.go

@@ -113,6 +116,10 @@ type Environment struct {
 	// values are merged.
 	CRDDirectoryPaths []string

+	// AbsBinaryDirectory is the path where the binaries required for the envtest are
+	// locate in the environment
+	AbsBinaryDirectory string


This name is a bit unclear. Can we make this TestBinariesDirectory or BinaryAssetsDirectory or something?

camilamacedo86 · 2020-11-23T11:24:07Z

The suggestions made are applied now.

estroz · 2020-11-23T18:06:34Z

pkg/envtest/server.go

-	assetPath := os.Getenv(envKubebuilderPath)
-	if assetPath == "" {
-		assetPath = defaultKubebuilderPath
+// getBinAssetPath will return the path for the binary informed


Suggested change

// getBinAssetPath will return the path for the binary informed

// getBinAssetPath returns a path for binary from the following list of locations,

// ordered by precedence:

// 1. KUBEBUILDER_ASSETS

// 2. Environment.BinaryAssetsDirectory

// 3. The default path, "/usr/local/kubebuilder/bin"

estroz · 2020-11-23T18:07:01Z

pkg/envtest/server.go

@@ -113,6 +117,10 @@ type Environment struct {
 	// values are merged.
 	CRDDirectoryPaths []string

+	// BinaryAssetsDirectory is the path where the binaries required for the envtest are
+	// locate in the environment


Suggested change

// locate in the environment

// located in the local environment. This field can be overridden by setting KUBEBUILDER_ASSETS.

camilamacedo86 · 2020-11-23T18:52:53Z

Hi @estroz,

Thank you for the review. All done 👍

estroz

/lgtm

DirectXMan12 · 2020-11-23T21:07:23Z

/approve

k8s-ci-robot · 2020-11-23T21:07:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, coderanger, DirectXMan12, estroz

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~~ [DirectXMan12]

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 the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 10, 2020

k8s-ci-robot requested review from alenkacz and shawn-hurley October 10, 2020 14:06

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 10, 2020

camilamacedo86 force-pushed the envtest-bin branch 2 times, most recently from 1ca5a31 to aa49d4f Compare October 10, 2020 14:17

camilamacedo86 removed request for alenkacz and shawn-hurley October 10, 2020 14:20

camilamacedo86 closed this Oct 10, 2020

camilamacedo86 reopened this Oct 12, 2020

camilamacedo86 requested review from mengqiy and mariantalla and removed request for mengqiy October 12, 2020 14:58

camilamacedo86 force-pushed the envtest-bin branch from aa49d4f to 8347433 Compare October 17, 2020 13:05

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 17, 2020

camilamacedo86 mentioned this pull request Oct 17, 2020

✨ add kubebuilder/tooling and controller setup to use binaries from the project bin (go/v3-alpha) kubernetes-sigs/kubebuilder#1711

Closed

camilamacedo86 force-pushed the envtest-bin branch from 8347433 to 275d18a Compare October 17, 2020 13:18

camilamacedo86 requested a review from DirectXMan12 October 17, 2020 13:18

camilamacedo86 assigned DirectXMan12 Oct 17, 2020

camilamacedo86 force-pushed the envtest-bin branch from 275d18a to 3b86f6a Compare October 17, 2020 13:51

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 17, 2020

camilamacedo86 force-pushed the envtest-bin branch 4 times, most recently from 8450022 to 8d02d33 Compare October 17, 2020 14:08

camilamacedo86 changed the title ~~✨ allow to pass the assets path via the environment config~~ ✨ Allow pass the assets path bin via the env config Nov 11, 2020

camilamacedo86 changed the title ~~✨ Allow pass the assets path bin via the env config~~ ✨ Allow to pass the assets path bin via the env config Nov 11, 2020

camilamacedo86 changed the title ~~✨ Allow to pass the assets path bin via the env config~~ ✨ Allow passing the assets path bin via the env config Nov 11, 2020

camilamacedo86 force-pushed the envtest-bin branch from ae9401c to c656739 Compare November 13, 2020 11:04

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 13, 2020

camilamacedo86 force-pushed the envtest-bin branch 3 times, most recently from 362e9ad to 24a75dd Compare November 13, 2020 11:48

DirectXMan12 suggested changes Nov 19, 2020

View reviewed changes

camilamacedo86 force-pushed the envtest-bin branch 2 times, most recently from 1c6bf85 to 07c7a67 Compare November 23, 2020 11:23

camilamacedo86 requested review from estroz and DirectXMan12 November 23, 2020 11:23

estroz reviewed Nov 23, 2020

View reviewed changes

✨ allow pass the assets path via the enviroment config

21c6666

camilamacedo86 force-pushed the envtest-bin branch from 07c7a67 to 21c6666 Compare November 23, 2020 18:52

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 23, 2020

camilamacedo86 requested a review from estroz November 23, 2020 18:52

estroz approved these changes Nov 23, 2020

View reviewed changes

k8s-ci-robot assigned estroz Nov 23, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 23, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 23, 2020

k8s-ci-robot merged commit f3831c2 into kubernetes-sigs:master Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Allow passing the assets path bin via the env config #1214

✨ Allow passing the assets path bin via the env config #1214

camilamacedo86 commented Oct 10, 2020 •

edited

camilamacedo86 commented Oct 12, 2020

camilamacedo86 commented Oct 17, 2020

DirectXMan12 left a comment •

edited

DirectXMan12 Nov 19, 2020

camilamacedo86 Nov 23, 2020

camilamacedo86 commented Nov 23, 2020

estroz Nov 23, 2020

estroz Nov 23, 2020 •

edited

camilamacedo86 commented Nov 23, 2020

estroz left a comment

DirectXMan12 commented Nov 23, 2020

k8s-ci-robot commented Nov 23, 2020

-// getBinAssetPath will return the path for the binary informed
+// getBinAssetPath returns a path for binary from the following list of locations,
+// ordered by precedence:
+// 1. KUBEBUILDER_ASSETS
+// 2. Environment.BinaryAssetsDirectory
+// 3. The default path, "/usr/local/kubebuilder/bin"

	// locate in the environment
	// located in the local environment. This field can be overridden by setting KUBEBUILDER_ASSETS.

✨ Allow passing the assets path bin via the env config #1214

✨ Allow passing the assets path bin via the env config #1214

Conversation

camilamacedo86 commented Oct 10, 2020 • edited

camilamacedo86 commented Oct 12, 2020

camilamacedo86 commented Oct 17, 2020

DirectXMan12 left a comment • edited

Choose a reason for hiding this comment

DirectXMan12 Nov 19, 2020

Choose a reason for hiding this comment

camilamacedo86 Nov 23, 2020

Choose a reason for hiding this comment

camilamacedo86 commented Nov 23, 2020

estroz Nov 23, 2020

Choose a reason for hiding this comment

estroz Nov 23, 2020 • edited

Choose a reason for hiding this comment

camilamacedo86 commented Nov 23, 2020

estroz left a comment

Choose a reason for hiding this comment

DirectXMan12 commented Nov 23, 2020

k8s-ci-robot commented Nov 23, 2020

camilamacedo86 commented Oct 10, 2020 •

edited

DirectXMan12 left a comment •

edited

estroz Nov 23, 2020 •

edited