-
Notifications
You must be signed in to change notification settings - Fork 1.6k
✨(feat): add necessary changes to support applyconfiguration gen #5237
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
base: master
Are you sure you want to change the base?
Changes from all commits
7166a19
8d51a9f
473f3ca
c91bfa9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,6 +100,7 @@ func (s *apiScaffolder) Scaffold() error { | |
| if err := scaffold.Execute( | ||
| &api.Types{Force: s.force}, | ||
| &api.Group{}, | ||
| &api.Doc{}, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should only generate the new files if the/when the flag is informed |
||
| ); err != nil { | ||
| return fmt.Errorf("error scaffolding APIs: %w", err) | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,59 @@ | ||
| /* | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for looking into that. 🎉 If it turns out to be a breaking change, we need to find a way to make it backwards compatible. We can’t introduce it right now. However, if you want to go ahead and implement the changes based on how you think it should work, and then share your findings with us, we can refine it together in a follow-up. Because of that, I wouldn’t worry about the e2e tests at this stage. Instead, focus on making the changes in a way that, when you run make generate, the testdata samples are regenerated—or at least show how a project would look with the new behavior. |
||
| Copyright 2022 The Kubernetes Authors. | ||
| Licensed under the Apache License, Version 2.0 (the "License"); | ||
| you may not use this file except in compliance with the License. | ||
| You may obtain a copy of the License at | ||
| http://www.apache.org/licenses/LICENSE-2.0 | ||
| Unless required by applicable law or agreed to in writing, software | ||
| distributed under the License is distributed on an "AS IS" BASIS, | ||
| WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| See the License for the specific language governing permissions and | ||
| limitations under the License. | ||
| */ | ||
|
|
||
| package api | ||
|
|
||
| import ( | ||
| log "log/slog" | ||
| "path/filepath" | ||
|
|
||
| "sigs.k8s.io/kubebuilder/v4/pkg/machinery" | ||
| ) | ||
|
|
||
| var _ machinery.Template = &Doc{} | ||
|
|
||
| // Doc scaffolds the doc file that defines the groupName | ||
| type Doc struct { | ||
| machinery.TemplateMixin | ||
| machinery.MultiGroupMixin | ||
| machinery.BoilerplateMixin | ||
| machinery.ResourceMixin | ||
| } | ||
|
|
||
| // SetTemplateDefaults implements machinery.Template | ||
| func (f *Doc) SetTemplateDefaults() error { | ||
| if f.Path == "" { | ||
| if f.MultiGroup && f.Resource.Group != "" { | ||
| f.Path = filepath.Join("api", "%[group]", "%[version]", "doc.go") | ||
| } else { | ||
| f.Path = filepath.Join("api", "%[version]", "doc.go") | ||
| } | ||
| } | ||
|
|
||
| f.Path = f.Resource.Replacer().Replace(f.Path) | ||
| log.Info(f.Path) | ||
| f.TemplateBody = docTemplate | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| //nolint:lll | ||
| const docTemplate = `{{ .Boilerplate }} | ||
| // Package {{ .Resource.Version }} contains API Schema definitions for the {{ .Resource.Group }} {{ .Resource.Version }} API group. | ||
| // +groupName={{ .Resource.QualifiedGroup }} | ||
| package {{ .Resource.Version }} | ||
| ` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -64,6 +64,11 @@ func (f *Types) SetTemplateDefaults() error { | |
| //nolint:lll | ||
| const typesTemplate = `{{ .Boilerplate }} | ||
|
|
||
| {{ if .Resource.HasApplyConfiguration }} | ||
| // +kubebuilder:ac:generate=true | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the feature must be optional.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I'll make it optional
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But if you want to add it in a way that we can first see it working, then we can evaluate it. Do you have a project that uses this configuration? I’ve never tried to use it myself, so looking at an example would be helpful and could give us some ideas on how we might add it.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a project that uses this configuration, however I ran the generate testdata using it: 537f54e
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the flag, made it true by default (for now so we can see the generated code) and also generated the code 👍 |
||
| {{ else }} | ||
| // +kubebuilder:ac:generate=false | ||
| {{ end }} | ||
| package {{ .Resource.Version }} | ||
|
|
||
| import ( | ||
|
|
@@ -116,6 +121,8 @@ type {{ .Resource.Kind }}Status struct { | |
| // +kubebuilder:resource:scope=Cluster | ||
| {{- else if not .Resource.IsRegularPlural }} | ||
| // +kubebuilder:resource:path={{ .Resource.Plural }} | ||
| {{- else }} | ||
| // +kubebuilder:resource | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What does this marker do?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. controller-tools uses this marker to mark the type as a resource type: Marker: https://github.com/kubernetes-sigs/controller-tools/blob/main/pkg/applyconfiguration/gen.go#L45C1-L45C127 |
||
| {{- end }} | ||
|
|
||
| // {{ .Resource.Kind }} is the Schema for the {{ .Resource.Plural }} API | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -123,9 +123,9 @@ manifests: controller-gen ## Generate WebhookConfiguration, ClusterRole and Cust | |
| .PHONY: generate | ||
| generate: controller-gen ## Generate code containing DeepCopy, DeepCopyInto, and DeepCopyObject method implementations. | ||
| {{ if .BoilerplatePath -}} | ||
| "$(CONTROLLER_GEN)" object:headerFile={{printf "%q" .BoilerplatePath}} paths="./..." | ||
| "$(CONTROLLER_GEN)" applyconfiguration:headerFile="hack/boilerplate.go.txt" object:headerFile={{printf "%q" .BoilerplatePath}} paths="./..." | ||
| {{- else -}} | ||
| "$(CONTROLLER_GEN)" object paths="./..." | ||
| "$(CONTROLLER_GEN)" applyconfiguration object paths="./..." | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, here we should not do this change as well. That would mean all would only work with. So, the first question is: In a project, can I have both? Apis created with applyconfiguration and others not? |
||
| {{- end }} | ||
|
|
||
| .PHONY: fmt | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can it be done per API or the whole project need to be changed to adopted this?
I mean, can I have 3 APIs that uses applyconfiguration and 3 that do not use them in the same project?
Is that possible?