Optionally generate an init func for an informer #2989

skonto · 2024-03-14T11:36:07Z

Changes

Allows to kip the generation of the init func for an informer
Based on the suggestion here Provide option to exclude a registered informer from starting #2976 (comment)
Replacement for Provide option to exclude a registered informer from starting #2976
Tested locally and here: [wip] Integrate net-certmanager in Serving serving#14955.
It can be used as follows:

 Knative Injection (for cert-manager)
${KNATIVE_CODEGEN_PKG}/hack/generate-knative.sh "injection" \
  knative.dev/serving/pkg/net-certmanager/client/certmanager github.com/cert-manager/cert-manager/pkg/apis \
  "certmanager:v1 acme:v1" \
  --skipInitFuncForInformer \
  --go-header-file ${REPO_ROOT_DIR}/hack/boilerplate/boilerplate.go.txt

and then on demand register the right informer:

if shouldEnableNetCertManagerController(ctx, client) {
		injection.Default.RegisterInformer(challenge.WithInformer)
		injection.Default.RegisterInformer(v1certificate.WithInformer)
		injection.Default.RegisterInformer(certificaterequest.WithInformer)
		injection.Default.RegisterInformer(clusterissuer.WithInformer)
		injection.Default.RegisterInformer(issuer.WithInformer)
		ctors = append(ctors, certificate.NewController) // add the net-certmanager controller
	}

We could have used a tag to skip per type but these are external types. In the feature we can do this per type for our
types eg. +skipInitInformer and thus pass this directly to the InformerGenerator.
We could expand it to filteredInformers if we want.

/assign @dprotaso

/kind enhancement

Release Note

Docs

codecov · 2024-03-14T11:54:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.68%. Comparing base (adefab4) to head (4a9843c).
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2989   +/-   ##
=======================================
  Coverage   78.68%   78.68%           
=======================================
  Files         188      188           
  Lines       11051    11051           
=======================================
  Hits         8695     8695           
  Misses       2092     2092           
  Partials      264      264

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

dprotaso · 2024-03-14T14:15:28Z

codegen/cmd/injection-gen/main.go

@@ -36,13 +37,17 @@ func main() {
 	genericArgs.AddFlags(pflag.CommandLine)
 	customArgs.AddFlags(pflag.CommandLine)
 	flag.Set("logtostderr", "true")
+	skipInitFuncForInformer := flag.Bool("skipInitFuncForInformer", false, "Skip the init function for informer")


Args can be added here: https://github.com/knative/pkg/blob/main/codegen/cmd/injection-gen/args/args.go

Cool, I was not sure if I should touch those.

dprotaso · 2024-03-14T14:15:40Z

codegen/cmd/injection-gen/main.go

+	if *skipInitFuncForInformer {
+		os.Setenv(generators.SkipInitFuncForInformerEnv, "true")
+	}


Not needed if we setup args properly

dprotaso · 2024-03-14T14:16:08Z

codegen/cmd/injection-gen/generators/informer.go

+	if os.Getenv(SkipInitFuncForInformerEnv) == "true" {
+		sw.Do(strings.ReplaceAll(injectionInformer, "withInformer", "WithInformer"), m)
+	} else {
+		sw.Do(initFunc+injectionInformer, m)
+	}


Can we pass the flag value to the context map m and just do the if logic in the template?

Let me try.

Ok I didn't see it was using go templates behind the scenes, now it makes sense.

skonto · 2024-03-26T13:07:24Z

@dprotaso gentle ping

dprotaso · 2024-03-26T16:49:36Z

codegen/cmd/injection-gen/args/args.go

@@ -49,6 +49,8 @@ func (ca *CustomArgs) AddFlags(fs *pflag.FlagSet) {

 	fs.BoolVar(&ca.ListerHasPointerElem, "lister-has-pointer-elem", false, "")
 	fs.MarkDeprecated("lister-has-pointer-elem", "this flag has no effect")
+
+	fs.Bool("skipInitFuncForInformer", false, "Skip the init function for informer")


We should wire this to the CustomArgs structs like we do for the other flags

Also can we follow the flag naming convention we are using which is snake-case

I'd probably call this disable-informer-init

@dprotaso ok done

skonto · 2024-03-27T15:54:37Z

@dprotaso pls take another look, thanks for the review!

dprotaso · 2024-03-27T21:50:59Z

codegen/cmd/injection-gen/generators/informer.go

@@ -78,6 +80,8 @@ func (g *injectionGenerator) GenerateType(c *generator.Context, t *types.Type, w

 	klog.V(5).Info("processing type ", t)

+	disableInformerInit, _ := pflag.CommandLine.GetBool("disable-informer-init")


we should be pulling this from the custom args struct and the funneling it into this generator

pkg/codegen/cmd/injection-gen/generators/packages.go

Line 41 in b8b7ca1

customArgs, ok := arguments.CustomArgs.(*informergenargs.CustomArgs)

pkg/codegen/cmd/injection-gen/generators/packages.go

Line 97 in b8b7ca1

packageList = append(packageList, versionInformerPackages(versionPackagePath, groupPackageName, gv, groupGoNames[groupPackageName], boilerplate, typesWithInformers, customArgs)...)

otherwise it's like we're reaching for globals.

The logic in packages is a bit verbose :/

@dprotaso done.

skonto · 2024-03-28T10:02:28Z

@dprotaso ready, could you stamp it?

dprotaso · 2024-03-28T13:49:19Z

/lgtm
/approve

thanks @skonto

knative-prow · 2024-03-28T13:49:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dprotaso,skonto]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Optionally generate an init func for an informer

c3e0944

knative-prow bot assigned dprotaso Mar 14, 2024

knative-prow bot added kind/enhancement approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 14, 2024

knative-prow bot requested review from creydr and ReToCode March 14, 2024 11:36

knative-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 14, 2024

skonto requested review from dprotaso and removed request for creydr March 14, 2024 11:36

lint

f6b0378

dprotaso reviewed Mar 14, 2024

View reviewed changes

skonto mentioned this pull request Mar 19, 2024

Provide option to exclude a registered informer from starting #2976

Closed

move condition to go template

913ecad

dprotaso reviewed Mar 26, 2024

View reviewed changes

rename flag and fixes

37ac625

dprotaso reviewed Mar 27, 2024

View reviewed changes

pass flag value via the generator

4a9843c

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2024

knative-prow bot merged commit f69f148 into knative:main Mar 28, 2024
40 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optionally generate an init func for an informer #2989

Optionally generate an init func for an informer #2989

skonto commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

dprotaso Mar 14, 2024

skonto Mar 19, 2024

dprotaso Mar 14, 2024

dprotaso Mar 14, 2024

skonto Mar 19, 2024 •

edited

Loading

skonto Mar 19, 2024 •

edited

Loading

skonto commented Mar 26, 2024

dprotaso Mar 26, 2024

skonto Mar 27, 2024

skonto commented Mar 27, 2024

dprotaso Mar 27, 2024

skonto Mar 28, 2024

skonto commented Mar 28, 2024

dprotaso commented Mar 28, 2024

knative-prow bot commented Mar 28, 2024

		@@ -78,6 +80,8 @@ func (g injectionGenerator) GenerateType(c generator.Context, t *types.Type, w

		klog.V(5).Info("processing type ", t)

		disableInformerInit, _ := pflag.CommandLine.GetBool("disable-informer-init")

Optionally generate an init func for an informer #2989

Optionally generate an init func for an informer #2989

Conversation

skonto commented Mar 14, 2024 • edited Loading

Changes

codecov bot commented Mar 14, 2024 • edited Loading

Codecov Report

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

skonto Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

skonto Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

skonto commented Mar 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Mar 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skonto commented Mar 28, 2024

dprotaso commented Mar 28, 2024

knative-prow bot commented Mar 28, 2024

skonto commented Mar 14, 2024 •

edited

Loading

codecov bot commented Mar 14, 2024 •

edited

Loading

skonto Mar 19, 2024 •

edited

Loading

skonto Mar 19, 2024 •

edited

Loading