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.

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

upgrade kubebuilder to plugin/v4 #418

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Conversation

randytqwjp
Copy link
Collaborator

What this PR does / why we need it:

In place upgrade for kubebuilder to plugin v4

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

@randytqwjp
Copy link
Collaborator Author

Tests currently failing will take a look

@sanposhiho sanposhiho marked this pull request as draft October 10, 2024 07:30
@randytqwjp
Copy link
Collaborator Author

@sanposhiho After upgrading kubebuilder, controller with name tortoise already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric. This error appears for every controller test after the first test. The way the controller tests are written for tortoise is very different from the documentation so i'm not sure how to fix this.. do you know if its possible to delete this controller in the cleanup?

@sanposhiho
Copy link
Collaborator

What if you move startController() like this?

	runTest := func(path string) {
		initializeResourcesFromFiles(ctx, k8sClient, filepath.Join(path, "before"))
-		stopFunc = startController(ctx)
		if os.Getenv("UPDATE_TESTCASES") == "true" {
			generateTestCases(path)
		} else {
			checkWithWantedResources(path)
		}
		cleanUp()
	}
+	BeforeEach(func() {
+		stopFunc = startController(ctx)
+	})

@randytqwjp
Copy link
Collaborator Author

@sanposhiho I fixed it with this m, err := manager.New(cfg, manager.Options{ Controller: config.Controller{ SkipNameValidation: ptr.To(true), }, }) apparently theres a new option to skip name validation. Maybe this was not present in previous controller-runtime version

@randytqwjp
Copy link
Collaborator Author

@sanposhiho Not sure what this exec error is for the tortoisectl test would you have any clue?


import "github.com/mercari/tortoise/cmd/tortoisectl/commands"

func main() {
func tortoisectl() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why renamed like this? It might be the reason

@randytqwjp
Copy link
Collaborator Author

@sanposhiho all the tests are passing but somehow lint is getting killed on the runner.. likely to be OOM? seems like the linter is using a lot of memory on the runner compared to my local

@randytqwjp
Copy link
Collaborator Author

@sanposhiho all the tests are passing but somehow lint is getting killed on the runner.. likely to be OOM? seems like the linter is using a lot of memory on the runner compared to my local

Looks like go version issue. Specified go1.22.6 and it works now

@randytqwjp randytqwjp marked this pull request as ready for review October 23, 2024 10:05
@@ -120,8 +120,6 @@ func main() {

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
Scheme: scheme,
MetricsBindAddress: metricsAddr,
Copy link
Collaborator

Choose a reason for hiding this comment

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

So, we no longer need a flag for metricAddr(L81)? How does it know the address?

@@ -73,6 +73,10 @@ func Test_TortoiseCtlStop(t *testing.T) {

// Build the latest binary.
buildTortoiseCtl(t)
_, _, err := execCommand(t, "chmod", "+x", "./testdata/bin/tortoisectl")
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe it's not needed, is it?

@@ -0,0 +1,122 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

@@ -0,0 +1,32 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@@ -0,0 +1,140 @@
/*
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants