Add missing clientgen markers #17

varshaprasad96 · 2022-05-19T23:47:25Z

Supersedes #14

Fixes: #10

varshaprasad96 · 2022-05-23T23:30:29Z

cc: @ncdc @fabianvf @njhale The PR is ready for review. With this, the additional genclient formats are now supported. Tested this out with some upstream k8s types:
For apps:v1 and certificates:v1, here is the output: https://github.com/varshaprasad96/kubernetes/tree/test-scaffolding/staging/src/k8s.io/client-go/clusterclient/typed

Unfortunately the entire k8s api don't work in one go. Digging into the upstream k8s code-gen, realized they have a patch to add plural names for the types which needs to be brought in here. I'll create an issue for that and fix it in follow up. (#19)

fabianvf · 2022-05-24T20:18:34Z

part of kcp-dev/kcp#759

stevekuznetsov · 2022-05-26T15:55:04Z

examples/pkg/clusterclient/typed/example/v1/examplev1.go

@@ -78,76 +77,76 @@ func (w *wrappedClusterTestType) checkCluster(ctx context.Context) (context.Cont
 	return ctx, nil
 }

-// Create implements ClusterTestTypeInterface.


Can we sort the methods being generated, perhaps adding first a commit to sort the previous generated output, so that this diff is legible, please?

The generated methods aren't sorted but in the order as specified here which is the order that code-gen follows. And previously the methods were hardcoded in templates instead of being split as done here. Really sorry for the clumsy diff :(

stevekuznetsov · 2022-05-26T15:56:25Z

pkg/generators/clientgen/gen.go

+	noVerbsMarker       = markers.Must(markers.MakeDefinition("genclient:noVerbs", markers.DescribesType, struct{}{}))
+	readOnlyMarker      = markers.Must(markers.MakeDefinition("genclient:readonly", markers.DescribesType, struct{}{}))
+
+	// These two markers are defined separately since they accept a list which is comma separated. In controller-tools, comma


What does "defined separately" mean? If I'm changing this file in the future, what do I need to make sure I do so nothing breaks here?

In controller-tool's terms the following format is how the marker is defined: <makername>:<parameter>=<values>. As a consumer of this library, I need to register (what MakeDefinition does here) just the marker name once. But there are 2 edge cases, because of which we are defining the marker formats separately.

The parameter does not take any value. That's why genclient:noVerbs, genclient:readonly are registered as separate markers.

The value accepts a list. Controller-tool's parsing logic identifies a list only if it is separated by ";", whereas genclinet formats have comma separated values. This confuses the parser, and screws up the logic. To work around this, we define this as separate markers.
Just as an example - this is the reason why genclient:Method marker is not defined explicitly, but others are.

So by "separately" you mean that we are passsing markers.RawArguments("")?

Not exactly, I meant that we are registering it to markers list explicitly using MakeDefinition (ie) this:

code-generator/pkg/generators/clientgen/gen.go

Lines 52 to 53 in d5edbfd

skipVerbsMarker = markers.Must(markers.MakeDefinition("genclient:skipVerbs", markers.DescribesType, markers.RawArguments("")))

onlyVerbsMarker = markers.Must(markers.MakeDefinition("genclient:onlyVerbs", markers.DescribesType, markers.RawArguments("")))

The method marker (ie) genclient:method doesn't need that kind explicit registration because it's considered as a sub-type of genclient marker. markers.RawArguments("") is actually just parsing whatever value is being passed to the respective marker (say genclient:onlyVerb) and returning to us.

Ah, I see - comparing everything here (from nonNamespacedMarker down) to ones that exist but are not even listed. I see. I was trying to tell the difference between e.g. these two under the comment and the ones above them.

I have modified the comments for these markers to make it a bit more clear.

stevekuznetsov

Overall, it does not seem like we have a clear distinction between data aggregation and output formatting. IMO when using Go templates, we should aggregate data in the language and format it in the template.

stevekuznetsov · 2022-05-26T15:58:23Z

pkg/generators/clientgen/gen.go

@@ -357,16 +380,14 @@ func (g *Generator) generateSubInterfaces(ctx *genall.GenerationContext) error {
 			// this is to accomodate multiple types defined in single group
 			byType := make(map[string][]byte)

+			importList := make([]string, 0)


Can we make appending to this an explicit thing instead of passing a pointer and doing an implicit/side-effect mutation?

The idea was to allow the subsequent functions (like scaffolding APIs) to modify the import list which is why it is passed as pointer. We could pass it explicitly and do an aggregation in the end. But wondering if that is required?

I think passing mutable state around to have side-effects change it makes for code that's hard to reason about, but we can figure it out in a follow-up.

stevekuznetsov · 2022-05-26T15:59:54Z

pkg/generators/clientgen/gen.go

+
+				namespaceScoped := info.Markers.Get(nonNamespacedMarker.Name) == nil
+				noVerbs := info.Markers.Get(noVerbsMarker.Name) != nil
+				hasStatus := hasStatusSubresource(info)


Why did we get rid of the other helpers, but not this one?

That's because it does the additional task of checking whether the resource field is present for the type or not -

code-generator/pkg/generators/clientgen/gen.go

Lines 452 to 457 in 4977dcf

for _, f := range info.Fields {

if f.Name == "Status" {

hasStatusField = true

break

}

}

, in addition to checking if the marker exists.

Do we emit some sort of validation error when a user asks for a status update but their type does not have it?

Nope, that condition doesn't arise. Because this is a noStatus marker. If

noStatus marker is present and status subfield is present or not present - we don't scaffold status sub resources.

noStatus marker is not present - if Status field is there its scaffolded, if not its not scaffolded.

pkg/generators/clientgen/gen.go

stevekuznetsov · 2022-05-26T16:11:04Z

pkg/internal/templates.go

-	{{.Name}}{{.Version}} "{{.ClientPath}}/typed/{{.Name}}/{{.Version}}"
-
-	kcp "github.com/kcp-dev/apimachinery/pkg/client"
-	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"


Since this is part of appendGenericImports, do I understand correctly that these are always imported? Why move this logic into Go from the template?

Yes, just to have everything added in a same place. Could be either ways.

pkg/internal/templates.go

pkg/util/util.go

njhale · 2022-05-26T18:58:02Z

main.go

@@ -17,11 +17,13 @@ limitations under the License.
 package main

 import (
+	goflags "flag"


nit: this is probably an indication that we either shouldn't use the name flag OR the flags should be exported from another package; e.g. pkg/generators

We have flags imported internally here:

code-generator/main.go

Line 28 in 4977dcf

"github.com/kcp-dev/code-generator/pkg/flag"

. Did you mean to rename those instead of tagging this as go flags?

main.go

njhale · 2022-05-26T20:19:01Z

pkg/internal/parser.go

+	PkgNameUpperFirst string
+	VersionUpperFirst string
+	NameLowerFirst    string


You can expose formatting methods to be used directly in the Go template ala https://github.com/njhale/code-generator/blob/77c28e0b5b75e66b76ec77e7ca1379871c96bdeb/pkg/internal/informergen/parser.go#L73 and https://github.com/njhale/code-generator/blob/77c28e0b5b75e66b76ec77e7ca1379871c96bdeb/pkg/internal/informergen/templates.go#L39

Yeah, that's what I was planning to do here in a follow-up: bf72d7f#diff-7cd18745c2d89da8fe056843208f0d785327606243491e7044af3bd0e4dedaf1R387-R395. Removing all the formatting from structs would clear up the code.

njhale · 2022-05-26T20:21:32Z

pkg/internal/parser.go

+		util.ImportFormat(fmt.Sprintf("%s%s", p.Name, p.Version), fmt.Sprintf("%s/typed/%s/%s", p.ClientPath, p.Name, p.Version)))
+}
+
+func adjustTemplate(template, verb string) string {


the conditionals can be in the template itself, no?

njhale · 2022-05-26T20:26:11Z

pkg/internal/parser.go

+	if a.hasVerb("get") {
+		if err := templateExecute(getTemplate, *a); err != nil {
+			return err
+		}
+	}
+
+	if a.hasVerb("list") {
+		if err := templateExecute(listTemplate, *a); err != nil {
+			return err
+		}
+	}
+
+	if a.hasVerb("watch") {
+		if err := templateExecute(watchTemplate, *a); err != nil {
+			return err
+		}
+	}
+
+	if a.hasVerb("create") {
+		if err := templateExecute(createTemplate, *a); err != nil {
+			return err
+		}
+	}
+
+	if a.hasVerb("update") {
+		if err := templateExecute(updateTemplate, *a); err != nil {
+			return err
+		}
+	}


Can't the conditionals for this live in the template itself?

Explained here: #17 (comment)

njhale · 2022-05-26T20:27:55Z

pkg/internal/parser.go

+	}
+
+	if a.hasVerb("apply") && generateApply {
+		*a.importList = append(*a.importList, util.ImportFormat(fmt.Sprintf("%sapply%s", a.PkgName, a.Version), fmt.Sprintf("%s/%s/%s", a.ApplyConfigurationPackage, a.PkgName, a.Version)))


It doesn't look like we need this to be a slice pointer.

If you want to mutate it in these callers, you need the pointer.

njhale · 2022-05-26T20:31:25Z

pkg/internal/templates.go

@@ -166,45 +157,86 @@ func (w *wrapped{{.Name}}) checkCluster(ctx context.Context) (context.Context, e
 	}
 	return ctx, nil
 }
+`
+const patchTemplate = `


I think we can join all these templates and add conditionals around them, no?

The reasons why I split the templates was:

Wanted to offload some of the conditional logic to go code, so that templates don't look clumsy.

Most importantly, the additionalMethods (extensions as called by upstream code-gen) need modification of specific verbs by overriding their input and result type. Merging all of this into one will make it really difficult to implement those.

(2) was the most important reason to split the templates. That's the reason I am still inclined towards having them separate instead of making everything into one single block even though the side-effect is to have a few more if statements when individual verbs are to be scaffolded.

stevekuznetsov · 2022-06-03T15:58:18Z

This needs a rebase but is otherwise ready to merge, I think

Signed-off-by: Andy Goldstein <[email protected]>

by upstream k8s code-generator. Signed-off-by: varshaprasad96 <[email protected]>

varshaprasad96 force-pushed the add-missing-clientgen-markers branch 2 times, most recently from 95451bd to 0f782a4 Compare May 23, 2022 23:24

varshaprasad96 marked this pull request as ready for review May 23, 2022 23:24

varshaprasad96 mentioned this pull request May 23, 2022

Add support for plural names for types #19

Closed

stevekuznetsov reviewed May 26, 2022

View reviewed changes

varshaprasad96 force-pushed the add-missing-clientgen-markers branch from 0f782a4 to d5edbfd Compare May 26, 2022 18:49

njhale reviewed May 26, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

njhale reviewed May 26, 2022

View reviewed changes

main.go Outdated Show resolved Hide resolved

varshaprasad96 force-pushed the add-missing-clientgen-markers branch 2 times, most recently from 0c9a4d7 to 6d704e3 Compare May 26, 2022 19:07

njhale suggested changes May 26, 2022

View reviewed changes

ncdc mentioned this pull request May 31, 2022

WIP add missing clientgen markers #14

Closed

varshaprasad96 force-pushed the add-missing-clientgen-markers branch from 6d704e3 to 9bda2d0 Compare May 31, 2022 19:59

varshaprasad96 requested review from stevekuznetsov and njhale May 31, 2022 20:03

varshaprasad96 mentioned this pull request Jun 2, 2022

[Fix] Add plural naming formats in scaffolding #21

Merged

ncdc and others added 2 commits June 6, 2022 10:18

wip

e22f31c

Signed-off-by: Andy Goldstein <[email protected]>

This commit adds support to all the //+genclient markers as specified

c64c58b

by upstream k8s code-generator. Signed-off-by: varshaprasad96 <[email protected]>

varshaprasad96 force-pushed the add-missing-clientgen-markers branch from 9bda2d0 to c64c58b Compare June 6, 2022 21:54

stevekuznetsov approved these changes Jun 7, 2022

View reviewed changes

stevekuznetsov merged commit b7945e2 into kcp-dev:main Jun 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing clientgen markers #17

Add missing clientgen markers #17

varshaprasad96 commented May 19, 2022 •

edited

Loading

varshaprasad96 commented May 23, 2022 •

edited

Loading

fabianvf commented May 24, 2022

stevekuznetsov May 26, 2022

varshaprasad96 May 26, 2022

stevekuznetsov May 26, 2022 •

edited

Loading

varshaprasad96 May 26, 2022

stevekuznetsov May 26, 2022

varshaprasad96 May 26, 2022

stevekuznetsov May 27, 2022

varshaprasad96 May 31, 2022

stevekuznetsov left a comment

stevekuznetsov May 26, 2022

varshaprasad96 May 26, 2022

stevekuznetsov May 27, 2022

stevekuznetsov May 26, 2022

varshaprasad96 May 26, 2022

stevekuznetsov May 26, 2022

varshaprasad96 May 31, 2022

stevekuznetsov May 26, 2022

varshaprasad96 May 26, 2022

njhale May 26, 2022 •

edited

Loading

varshaprasad96 May 31, 2022

njhale May 26, 2022

varshaprasad96 May 27, 2022

njhale May 26, 2022

njhale May 26, 2022

varshaprasad96 May 31, 2022

njhale May 26, 2022

stevekuznetsov May 27, 2022

njhale May 26, 2022

varshaprasad96 May 27, 2022

varshaprasad96 May 27, 2022 •

edited

Loading

stevekuznetsov commented Jun 3, 2022

	skipVerbsMarker = markers.Must(markers.MakeDefinition("genclient:skipVerbs", markers.DescribesType, markers.RawArguments("")))
	onlyVerbsMarker = markers.Must(markers.MakeDefinition("genclient:onlyVerbs", markers.DescribesType, markers.RawArguments("")))

	for _, f := range info.Fields {
	if f.Name == "Status" {
	hasStatusField = true
	break
	}
	}

Add missing clientgen markers #17

Add missing clientgen markers #17

Conversation

varshaprasad96 commented May 19, 2022 • edited Loading

varshaprasad96 commented May 23, 2022 • edited Loading

fabianvf commented May 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov May 26, 2022 • 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

stevekuznetsov left a 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

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

njhale May 26, 2022 • 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

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

varshaprasad96 May 27, 2022 • edited Loading

Choose a reason for hiding this comment

stevekuznetsov commented Jun 3, 2022

varshaprasad96 commented May 19, 2022 •

edited

Loading

varshaprasad96 commented May 23, 2022 •

edited

Loading

stevekuznetsov May 26, 2022 •

edited

Loading

njhale May 26, 2022 •

edited

Loading

varshaprasad96 May 27, 2022 •

edited

Loading