Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP fix: standardize scaffold suite_test files #4441

Closed
wants to merge 2 commits into from
Closed

WIP fix: standardize scaffold suite_test files #4441

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
08d842a
fix: standardize scaffold tests
mateusoliveira43 Dec 26, 2024
5345ea8
fixup! fix: standardize scaffold tests
mateusoliveira43 Dec 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ Now, let's go through the code generated.
*/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit:

Any change that impacts the scaffolds will directly affect end users and, therefore, cannot be marked with 🌱. Please note that these emojis are used to help us generate the release notes. Both 🌱 and 📖 lead to items being listed only under the "All changes" section and not highlighted in the main release notes. While 🌱 is appropriate for test-related changes, in this case, since we're modifying the default scaffolds, it should not be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated PR title

var (
ctx context.Context
cancel context.CancelFunc
testEnv *envtest.Environment
cfg *rest.Config
k8sClient client.Client // You'll be using this client in your tests.
testEnv *envtest.Environment
)
var ctx context.Context
var cancel context.CancelFunc

func TestControllers(t *testing.T) {
RegisterFailHandler(Fail)
Expand All @@ -74,11 +74,26 @@ var _ = BeforeSuite(func() {

ctx, cancel = context.WithCancel(context.TODO())

var err error
/*
First, the autogenerated test code will add the CronJob Kind schema to the default client-go k8s scheme.
This ensures that the CronJob API/Kind will be used in our test controller.
mateusoliveira43 marked this conversation as resolved.
Show resolved Hide resolved
*/
err = batchv1.AddToScheme(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())
/*
After the schemas, you will see the following marker.
This marker is what allows new schemas to be added here automatically when a new API is added to the project.
*/

// +kubebuilder:scaffold:scheme

/*
First, the envtest cluster is configured to read CRDs from the CRD directory Kubebuilder scaffolds for you.
The envtest cluster is configured to read CRDs from the CRD directory Kubebuilder scaffolds for you.
mateusoliveira43 marked this conversation as resolved.
Show resolved Hide resolved
*/
By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDInstallOptions: envtest.CRDInstallOptions{Scheme: scheme.Scheme},
CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")},
ErrorIfCRDPathMissing: true,
}
Expand All @@ -91,25 +106,11 @@ var _ = BeforeSuite(func() {
Then, we start the envtest cluster.
*/

var err error
// cfg is defined in this file globally.
cfg, err = testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expect(cfg).NotTo(BeNil())

/*
The autogenerated test code will add the CronJob Kind schema to the default client-go k8s scheme.
This ensures that the CronJob API/Kind will be used in our test controller.
*/
err = batchv1.AddToScheme(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())
/*
After the schemas, you will see the following marker.
This marker is what allows new schemas to be added here automatically when a new API is added to the project.
*/

// +kubebuilder:scaffold:scheme

/*
A client is created for our test CRUD operations.
*/
Expand Down Expand Up @@ -155,7 +156,6 @@ var _ = BeforeSuite(func() {
err = k8sManager.Start(ctx)
Expect(err).ToNot(HaveOccurred(), "failed to run manager")
}()

})

/*
Expand Down
37 changes: 18 additions & 19 deletions docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/webhook_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,7 @@ import (
. "github.com/onsi/gomega"

admissionv1 "k8s.io/api/admission/v1"

batchv1 "tutorial.kubebuilder.io/project/api/v1"

// +kubebuilder:scaffold:imports
apimachineryruntime "k8s.io/apimachinery/pkg/runtime"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this one not used?
If so good catcher.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was

I removed because webhooks suite test used an "empty" scheme (with only user CRDs) and controllers suite test used a "fully loaded" scheme (with user CRDs and default objects loaded, like namespaces, pods, etc)

Without this change, users can not create a namespace in webhook envtest tests, for example

Copy link
Member

@camilamacedo86 camilamacedo86 Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see you removed

scheme := apimachineryruntime.NewScheme()
err = batchv1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

So, now all coretypes are added by default, right?

To ensure clarity and make it easier to follow the changes, I think we should split this into separate PRs for each goal. This approach will help in maintaining a clear discussion for each change, simplify the release notes, and ensure each fix or change gets the attention it deserves.

Could you please open a PR for each fix/change proposed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify: For example, in this case, we need to explain what has changed and why in the PR, and release notes so due that we cannot have many changes with diff purposes at the same PR

"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -43,17 +39,20 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log/zap"
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
"sigs.k8s.io/controller-runtime/pkg/webhook"

batchv1 "tutorial.kubebuilder.io/project/api/v1"
// +kubebuilder:scaffold:imports
)

// These tests use Ginkgo (BDD-style Go testing framework). Refer to
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.

var (
ctx context.Context
cancel context.CancelFunc
testEnv *envtest.Environment
cfg *rest.Config
ctx context.Context
k8sClient client.Client
testEnv *envtest.Environment
)

func TestAPIs(t *testing.T) {
Expand All @@ -67,8 +66,18 @@ var _ = BeforeSuite(func() {

ctx, cancel = context.WithCancel(context.TODO())

var err error
err = batchv1.AddToScheme(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())

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

// +kubebuilder:scaffold:scheme

By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDInstallOptions: envtest.CRDInstallOptions{Scheme: scheme.Scheme},
Copy link
Member

@camilamacedo86 camilamacedo86 Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this option was added?
I do not think that we need it by default?
Would that be required for ANY case scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is needed for webhook tests #4441 (comment)

I think only needed for webhook, but would add to both, for consistency

CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "config", "crd", "bases")},
ErrorIfCRDPathMissing: false,

Expand All @@ -82,29 +91,19 @@ var _ = BeforeSuite(func() {
testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir()
}

var err error
// cfg is defined in this file globally.
cfg, err = testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expect(cfg).NotTo(BeNil())

scheme := apimachineryruntime.NewScheme()
err = batchv1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

err = admissionv1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:scheme
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catcher


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

// start webhook server using Manager.
webhookInstallOptions := &testEnv.WebhookInstallOptions
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme,
Scheme: scheme.Scheme,
WebhookServer: webhook.NewServer(webhook.Options{
Host: webhookInstallOptions.LocalServingHost,
Port: webhookInstallOptions.LocalServingPort,
Expand Down
26 changes: 14 additions & 12 deletions docs/book/src/getting-started/testdata/project/internal/controller/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,13 @@ import (
// These tests use Ginkgo (BDD-style Go testing framework). Refer to
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.

var cfg *rest.Config
var k8sClient client.Client
var testEnv *envtest.Environment
var ctx context.Context
var cancel context.CancelFunc
var (
ctx context.Context
cancel context.CancelFunc
testEnv *envtest.Environment
cfg *rest.Config
k8sClient client.Client
)

func TestControllers(t *testing.T) {
RegisterFailHandler(Fail)
Expand All @@ -56,8 +58,15 @@ var _ = BeforeSuite(func() {

ctx, cancel = context.WithCancel(context.TODO())

var err error
err = cachev1alpha1.AddToScheme(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:scheme

By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDInstallOptions: envtest.CRDInstallOptions{Scheme: scheme.Scheme},
CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")},
ErrorIfCRDPathMissing: true,
}
Expand All @@ -67,21 +76,14 @@ var _ = BeforeSuite(func() {
testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir()
}

var err error
// cfg is defined in this file globally.
cfg, err = testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expect(cfg).NotTo(BeNil())

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

// +kubebuilder:scaffold:scheme

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

})

var _ = AfterSuite(func() {
Expand Down
38 changes: 19 additions & 19 deletions docs/book/src/multiversion-tutorial/testdata/project/internal/controller/suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ Now, let's go through the code generated.
*/

var (
ctx context.Context
cancel context.CancelFunc
testEnv *envtest.Environment
cfg *rest.Config
k8sClient client.Client // You'll be using this client in your tests.
testEnv *envtest.Environment
)
var ctx context.Context
var cancel context.CancelFunc

func TestControllers(t *testing.T) {
RegisterFailHandler(Fail)
Expand All @@ -74,11 +74,26 @@ var _ = BeforeSuite(func() {

ctx, cancel = context.WithCancel(context.TODO())

var err error
/*
First, the autogenerated test code will add the CronJob Kind schema to the default client-go k8s scheme.
This ensures that the CronJob API/Kind will be used in our test controller.
*/
err = batchv1.AddToScheme(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())
/*
After the schemas, you will see the following marker.
This marker is what allows new schemas to be added here automatically when a new API is added to the project.
*/

// +kubebuilder:scaffold:scheme

/*
First, the envtest cluster is configured to read CRDs from the CRD directory Kubebuilder scaffolds for you.
The envtest cluster is configured to read CRDs from the CRD directory Kubebuilder scaffolds for you.
*/
By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDInstallOptions: envtest.CRDInstallOptions{Scheme: scheme.Scheme},
CRDDirectoryPaths: []string{filepath.Join("..", "..", "config", "crd", "bases")},
ErrorIfCRDPathMissing: true,
}
Expand All @@ -91,25 +106,11 @@ var _ = BeforeSuite(func() {
Then, we start the envtest cluster.
*/

var err error
// cfg is defined in this file globally.
cfg, err = testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expect(cfg).NotTo(BeNil())

/*
The autogenerated test code will add the CronJob Kind schema to the default client-go k8s scheme.
This ensures that the CronJob API/Kind will be used in our test controller.
*/
err = batchv1.AddToScheme(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())
/*
After the schemas, you will see the following marker.
This marker is what allows new schemas to be added here automatically when a new API is added to the project.
*/

// +kubebuilder:scaffold:scheme

/*
A client is created for our test CRUD operations.
*/
Expand Down Expand Up @@ -155,7 +156,6 @@ var _ = BeforeSuite(func() {
err = k8sManager.Start(ctx)
Expect(err).ToNot(HaveOccurred(), "failed to run manager")
}()

})

/*
Expand Down
37 changes: 18 additions & 19 deletions ...book/src/multiversion-tutorial/testdata/project/internal/webhook/v1/webhook_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,7 @@ import (
. "github.com/onsi/gomega"

admissionv1 "k8s.io/api/admission/v1"

batchv1 "tutorial.kubebuilder.io/project/api/v1"

// +kubebuilder:scaffold:imports
apimachineryruntime "k8s.io/apimachinery/pkg/runtime"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand All @@ -43,17 +39,20 @@ import (
"sigs.k8s.io/controller-runtime/pkg/log/zap"
metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server"
"sigs.k8s.io/controller-runtime/pkg/webhook"

batchv1 "tutorial.kubebuilder.io/project/api/v1"
// +kubebuilder:scaffold:imports
)

// These tests use Ginkgo (BDD-style Go testing framework). Refer to
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.

var (
ctx context.Context
cancel context.CancelFunc
testEnv *envtest.Environment
cfg *rest.Config
ctx context.Context
k8sClient client.Client
testEnv *envtest.Environment
)

func TestAPIs(t *testing.T) {
Expand All @@ -67,8 +66,18 @@ var _ = BeforeSuite(func() {

ctx, cancel = context.WithCancel(context.TODO())

var err error
err = batchv1.AddToScheme(scheme.Scheme)
Expect(err).NotTo(HaveOccurred())

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

// +kubebuilder:scaffold:scheme

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

Expand All @@ -82,29 +91,19 @@ var _ = BeforeSuite(func() {
testEnv.BinaryAssetsDirectory = getFirstFoundEnvTestBinaryDir()
}

var err error
// cfg is defined in this file globally.
cfg, err = testEnv.Start()
Expect(err).NotTo(HaveOccurred())
Expect(cfg).NotTo(BeNil())

scheme := apimachineryruntime.NewScheme()
err = batchv1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

err = admissionv1.AddToScheme(scheme)
Expect(err).NotTo(HaveOccurred())

// +kubebuilder:scaffold:scheme

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

// start webhook server using Manager.
webhookInstallOptions := &testEnv.WebhookInstallOptions
mgr, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme,
Scheme: scheme.Scheme,
WebhookServer: webhook.NewServer(webhook.Options{
Host: webhookInstallOptions.LocalServingHost,
Port: webhookInstallOptions.LocalServingPort,
Expand Down
Loading
Loading