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

Add DeepCopy generation for additional types in builders #6016

Closed
killianmuldoon opened this issue Jan 28, 2022 · 12 comments · Fixed by #6764
Closed

Add DeepCopy generation for additional types in builders #6016

killianmuldoon opened this issue Jan 28, 2022 · 12 comments · Fixed by #6764
Assignees
Labels
area/testing Issues or PRs related to testing good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Milestone

Comments

@killianmuldoon
Copy link
Contributor

killianmuldoon commented Jan 28, 2022

#5987 Adds new DeepCopy types to some of the builders used in our tests. These methods allow the same builder to be used to generate multiple test objects which should reduce some of the copy-pasting in test code.

Some of the types in the builders file do not have DeepCopy methods generated yet. This is because they contain a field that is map[string]interface{} which the controller gen tool is not compatible with.

These types are currently excluded using the kubebuilder:object:generate=false tag. We should restructure these builder objects to allow them to use code generation by changing their map[string]interface{} to a type that controller-gen will be able to generate DeepCopy methods for e.g. apiextensionsv1.JSON{} and generate DeepCopy methods for those types.

c/f #5987 (comment)

/area testing
/good-first-issue

@k8s-ci-robot k8s-ci-robot added the area/testing Issues or PRs related to testing label Jan 28, 2022
@killianmuldoon
Copy link
Contributor Author

/good-first-issue

@k8s-ci-robot
Copy link
Contributor

@killianmuldoon:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

For more details on the requirements of such an issue, please see here and ensure that they are met.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/good-first-issue

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/test-infra repository.

@k8s-ci-robot k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Jan 28, 2022
@sayantani11
Copy link
Contributor

/assign

@vincepri
Copy link
Member

/milestone Next

@k8s-ci-robot k8s-ci-robot added this to the Next milestone Jan 31, 2022
@sbueringer
Copy link
Member

/milestone v1.2

@k8s-ci-robot k8s-ci-robot modified the milestones: Next, v1.2 Feb 18, 2022
@killianmuldoon
Copy link
Contributor Author

Hi @sayantani11 do you need any help / guidance with this?

@sayantani11
Copy link
Contributor

@killianmuldoon Yes!! I am still unsure about how to proceed with the issue, although I understood the issue

@killianmuldoon
Copy link
Contributor Author

killianmuldoon commented Mar 4, 2022

So the root issue is that we can't generate DeepCopy methods for any of our builders which contain a field with the format map[string]interface{}{}. There's a few ways we could go about trying to change this, but basically we need every field in the object to be DeepCopy() - able.

I think the simplest approach is to change the structure of the builder so that it holds a copy of the object it will eventually build internally instead of carrying a list of fields. The internalized object will always be possible to DeepCopy - even if it's from Kubernetes' unstructured.Unstructured.

For example, with InfrastructureMachineTemplateBuilder you can replace these lines with something like the following:

// InfrastructureMachineTemplateBuilder holds the variables and objects needed to build an InfrastructureMachineTemplate.
// +kubebuilder:object:generate=false
type InfrastructureMachineTemplateBuilder struct {
	obj *unstructured.Unstructured
}

// InfrastructureMachineTemplate creates an InfrastructureMachineTemplateBuilder with the given name and namespace.
func InfrastructureMachineTemplate(namespace, name string) *InfrastructureMachineTemplateBuilder {
	obj := &unstructured.Unstructured{}
	obj.SetName(name)
	obj.SetNamespace(namespace)
	obj.SetAPIVersion(InfrastructureGroupVersion.String())
	obj.SetKind(GenericInfrastructureMachineTemplateKind)
	return &InfrastructureMachineTemplateBuilder{
		obj,
	}
}

// WithSpecFields sets a map of spec fields on the unstructured object. The keys in the map represent the path and the value corresponds
// to the value of the spec field.
//
// Note: all the paths should start with "spec."
//
// Example map: map[string]interface{}{
//     "spec.version": "v1.2.3",
// }.
func (i *InfrastructureMachineTemplateBuilder) WithSpecFields(fields map[string]interface{}) *InfrastructureMachineTemplateBuilder {
	for field, value := range fields {
		unstructured.SetNestedField(i.obj.UnstructuredContent(), value, field)
	}
	return i
}

// Build takes the objects and variables in the  InfrastructureMachineTemplateBuilder and generates an unstructured object.
func (i *InfrastructureMachineTemplateBuilder) Build() *unstructured.Unstructured {
	if _, ok, _ := unstructured.NestedMap(i.obj.Object, "spec"); !ok {
		unstructured.SetNestedField(i.obj.Object, map[string]interface{}{}, "spec")
	}
	if _, ok, _ := unstructured.NestedMap(i.obj.Object, "spec.template.spec"); !ok {
		unstructured.SetNestedField(i.obj.Object, map[string]interface{}{}, "spec", "template", "spec")
	}

	return i.obj
}

I've put this together quickly so there's probably still some errors with this version, but it should give you a start. From here you'll have to repeat the process for the other types that currently have the // +kubebuilder:object:generate=false tag.

Whenever you'd like some input it would be a good idea to open a [WIP] PR and I can take a look - feel free to ask questions on here or on slack otherwise. Honestly this issue isn't as simple as I first thought - it took me a while to get to the solution above. 😄

@sayantani11
Copy link
Contributor

@killianmuldoon Sorry was caught up, I will open a WIP PR asap!

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 10, 2022
@sayantani11
Copy link
Contributor

/remove lifecycle-stale
I'll make another PR for the issue ASAP

@killianmuldoon
Copy link
Contributor Author

Thanks so much for your help with this work @sayantani11! 😃 I had a bit of time today so I've put out a PR to close the remaining deepcopy tasks in the builders package.

Thanks again for getting involved!

/unassign @sayantani11
/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/testing Issues or PRs related to testing good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale.
Projects
None yet
6 participants