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

fix(e2e): use generated API methods #3874

Merged
merged 2 commits into from
Dec 7, 2022
Merged

fix(e2e): use generated API methods #3874

merged 2 commits into from
Dec 7, 2022

Conversation

squakez
Copy link
Contributor

@squakez squakez commented Dec 2, 2022

Closes #3818

Release Note

fix(e2e): use generated API methods

69ded07 because the patch use case is already tested in "Scale integration with polymorphic client". The goal of this test looks to be correctly testing the behavior of a get and a set operation. If it keeps failing, we should investigate the reason why the update is failing instead.
@squakez
Copy link
Contributor Author

squakez commented Dec 5, 2022

@phantomjinx, I had a further look and it seems that the "patch" scenario is already tested in

_, err = scaleClient.Scales(ns).Patch(TestContext, v1.SchemeGroupVersion.WithResource("integrations"), name, types.MergePatchType, []byte(patch), metav1.PatchOptions{})

I've tried to enable the // +genclient:method=PatchScale,verb=patch,subresource=patch,result=k8s.io/api/autoscaling/v1.Scale but the generation is failing. Looking further on the test, it seems that the original goal of this test was to really test the getter and the setter of the autogenerated Integration Scale client. Is this correct @astefanutti? so, if it keeps failing we should find the reason why the autogenerated code is broken IMO.

Are you okey on reverting these changes? what was the "well known" behavior you were trying to fix?

@astefanutti
Copy link
Member

It should ideally be possible to patch the integration scale sub-resource via the generated client and find the right syntax to have the corresponding method generated.

@squakez
Copy link
Contributor Author

squakez commented Dec 7, 2022

It should ideally be possible to patch the integration scale sub-resource via the generated client and find the right syntax to have the corresponding method generated.

Yeah, it was the first thing I tried, but it fails with:

./script/gen_client.sh
Generating Go client code...
F1207 08:04:39.322604   40897 main.go:62] Error: Failed executing generator: some packages had errors:
template: /home/squake/go/pkg/mod/k8s.io/code-generator@v0.23.5/cmd/client-gen/generators/generator_for_type.go:200:1:99: executing "/home/squake/go/pkg/mod/k8s.io/code-generator@v0.23.5/cmd/client-gen/generators/generator_for_type.go:200" at <raw>: error calling raw: runtime error: invalid memory address or nil pointer dereference
goroutine 1 [running]:
k8s.io/klog/v2.stacks(0x1)
	/home/squake/go/pkg/mod/k8s.io/klog/v2@v2.30.0/klog.go:1038 +0x8a
k8s.io/klog/v2.(*loggingT).output(0xa20500, 0x3, 0x0, 0xc007799880, 0x0, {0x889a36, 0x1}, 0xc00717eb00, 0x0)
	/home/squake/go/pkg/mod/k8s.io/klog/v2@v2.30.0/klog.go:987 +0x5fd
k8s.io/klog/v2.(*loggingT).printf(0x6, 0x7ad830, 0x0, {0x0, 0x0}, {0x77642a, 0x9}, {0xc00717eb00, 0x1, 0x1})
	/home/squake/go/pkg/mod/k8s.io/klog/v2@v2.30.0/klog.go:753 +0x1c5
k8s.io/klog/v2.Fatalf(...)
	/home/squake/go/pkg/mod/k8s.io/klog/v2@v2.30.0/klog.go:1532
main.main()
	/home/squake/go/pkg/mod/k8s.io/code-generator@v0.23.5/cmd/client-gen/main.go:62 +0x307

goroutine 19 [chan receive]:
k8s.io/klog/v2.(*loggingT).flushDaemon(0x0)
	/home/squake/go/pkg/mod/k8s.io/klog/v2@v2.30.0/klog.go:1181 +0x6a
created by k8s.io/klog/v2.init.0
	/home/squake/go/pkg/mod/k8s.io/klog/v2@v2.30.0/klog.go:420 +0xfb

In any case, in theory, the update should be reliable, and in the test it seems it works correctly. For this reason I was trying to understand how to correctly leverage the autogenerated code for this test.

@squakez squakez merged commit eb4e029 into apache:main Dec 7, 2022
@squakez
Copy link
Contributor Author

squakez commented Dec 7, 2022

I need to merge this as it is blocking #3837 (the release process run a make generate that wipe off those funcs added manually there). We may want to keep discussing about this though to see how to apply those changes in an autogenerated fashion.

@squakez squakez deleted the fix/3818 branch December 7, 2022 09:54
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.

make generate removes PatchScale func
3 participants