WIP fix: standardize scaffold suite_test files #4441

mateusoliveira43 · 2024-12-26T17:09:40Z

Scaffold internal/controller/suite_test.go and internal/webhook/version/suite_test.go have very similar code.

This PR makes them more similar

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

k8s-ci-robot · 2024-12-26T17:09:47Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mateusoliveira43
Once this PR has been reviewed and has the lgtm label, please assign kavinjsir for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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 · 2024-12-26T17:09:50Z

Hi @mateusoliveira43. 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-sigs/prow repository.

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

mateusoliveira43 · 2024-12-27T16:39:12Z

pkg/plugins/golang/v4/scaffolds/internal/templates/webhooks/webhook_suitetest.go

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


made this change because of this kubernetes-sigs/controller-runtime#1882 (comment)

camilamacedo86 · 2024-12-27T17:04:12Z

docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go

@@ -56,12 +56,12 @@ Now, let's go through the code generated.
 */



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.

updated PR title

docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go

camilamacedo86 · 2024-12-27T17:12:47Z

docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/webhook_suite_test.go

-	batchv1 "tutorial.kubebuilder.io/project/api/v1"
-
-	// +kubebuilder:scaffold:imports
-	apimachineryruntime "k8s.io/apimachinery/pkg/runtime"


was this one not used?
If so good catcher.

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

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?

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

camilamacedo86 · 2024-12-27T17:13:28Z

docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/webhook_suite_test.go

-	err = admissionv1.AddToScheme(scheme)
-	Expect(err).NotTo(HaveOccurred())
-
-	// +kubebuilder:scaffold:scheme


Good catcher

camilamacedo86 · 2024-12-27T17:13:59Z

docs/book/src/multiversion-tutorial/testdata/project/internal/webhook/v2/webhook_suite_test.go

 	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,


Why this change is required?

same motivation as this one #4441 (comment)

camilamacedo86

HI @mateusoliveira43

This is great work 🥇 , but I think it would be beneficial to split these changes into separate PRs, each addressing a specific context. While the changes are relatively small, having focused PRs improves clarity and makes it easier to review and track changes.

I suggest organizing the changes as follows:

📖 Improve and fix the test information for tutorials
Focus on enhancing and correcting the tutorial content to provide clearer guidance for users.
(go/v4): 🐛 Ensure that schemes are scaffolded before the envtest starts
Address the issue where schemes need to be properly registered before the test environment is initialized to avoid potential runtime errors.
(go/v4): Standardize the webhook and controller test layout
Introduce a consistent layout for webhook and controller tests, aligning them with best practices for readability and maintainability.

Splitting these into distinct PRs will make each change easier to review and maintain. Let me know what you think!

camilamacedo86 · 2024-12-27T17:23:49Z

docs/book/src/cronjob-tutorial/testdata/project/internal/webhook/v1/webhook_suite_test.go

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


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

it is needed for webhook tests #4441 (comment)

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

k8s-ci-robot · 2024-12-27T19:50:44Z

PR needs rebase.

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-sigs/prow repository.

mateusoliveira43 · 2024-12-27T21:21:50Z

PR was split in 3:

fix: standardize scaffold tests

08d842a

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

k8s-ci-robot requested review from camilamacedo86 and Kavinjsir December 26, 2024 17:09

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 26, 2024

fixup! fix: standardize scaffold tests

5345ea8

Signed-off-by: Mateus Oliveira <msouzaol@redhat.com>

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Dec 27, 2024

mateusoliveira43 commented Dec 27, 2024

View reviewed changes

camilamacedo86 reviewed Dec 27, 2024

View reviewed changes

docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go Show resolved Hide resolved

camilamacedo86 reviewed Dec 27, 2024

View reviewed changes

docs/book/src/cronjob-tutorial/testdata/project/internal/controller/suite_test.go Show resolved Hide resolved

camilamacedo86 reviewed Dec 27, 2024

View reviewed changes

mateusoliveira43 changed the title ~~🌱 fix: standardize scaffold suite_test files~~ ✨ fix: standardize scaffold suite_test files Dec 27, 2024

camilamacedo86 requested changes Dec 27, 2024

View reviewed changes

camilamacedo86 reviewed Dec 27, 2024

View reviewed changes

mateusoliveira43 changed the title ~~✨ fix: standardize scaffold suite_test files~~ WIP fix: standardize scaffold suite_test files Dec 27, 2024

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Dec 27, 2024

mateusoliveira43 closed this Dec 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP fix: standardize scaffold suite_test files #4441

WIP fix: standardize scaffold suite_test files #4441

mateusoliveira43 commented Dec 26, 2024 •

edited

Loading

k8s-ci-robot commented Dec 26, 2024

k8s-ci-robot commented Dec 26, 2024

mateusoliveira43 Dec 27, 2024

camilamacedo86 Dec 27, 2024

mateusoliveira43 Dec 27, 2024

camilamacedo86 Dec 27, 2024

mateusoliveira43 Dec 27, 2024

camilamacedo86 Dec 27, 2024 •

edited

Loading

mateusoliveira43 Dec 27, 2024

camilamacedo86 Dec 27, 2024

camilamacedo86 Dec 27, 2024

camilamacedo86 Dec 27, 2024

mateusoliveira43 Dec 27, 2024

camilamacedo86 left a comment

camilamacedo86 Dec 27, 2024 •

edited

Loading

mateusoliveira43 Dec 27, 2024

k8s-ci-robot commented Dec 27, 2024

mateusoliveira43 commented Dec 27, 2024

		@@ -56,12 +56,12 @@ Now, let's go through the code generated.
		*/

WIP fix: standardize scaffold suite_test files #4441

WIP fix: standardize scaffold suite_test files #4441

Conversation

mateusoliveira43 commented Dec 26, 2024 • edited Loading

k8s-ci-robot commented Dec 26, 2024

k8s-ci-robot commented Dec 26, 2024

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 Dec 27, 2024 • 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

camilamacedo86 left a comment

Choose a reason for hiding this comment

camilamacedo86 Dec 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Dec 27, 2024

mateusoliveira43 commented Dec 27, 2024

mateusoliveira43 commented Dec 26, 2024 •

edited

Loading

camilamacedo86 Dec 27, 2024 •

edited

Loading

camilamacedo86 Dec 27, 2024 •

edited

Loading