[Code-generator] `--extra-peer-dirs` flag is not supported by new `kube-codegen.sh` script #120803

ary1992 · 2023-09-21T12:04:28Z

What would you like to be added?

Earlier it was possible to pass --extra-peer-dirs flag to conversion-gen while calling generate-internal-groups.sh script.
Now with the new kube-codegen.sh script, conversion-gen is called under kube::codegen::gen_helpers which doesnot support any other flags apart from --input-pkg-root, --output-base and --boilerplate.

As it can be seen here there is no option to pass flag to conversion-gen

 "${gobin}/conversion-gen" \
            -v "${v}" \
            -O zz_generated.conversion \
            --go-header-file "${boilerplate}" \
            --output-base "${out_base}" \
            "${inputs[@]}"

https://github.com/kubernetes/code-generator/blob/e4611069dfb4b0c04c7751afae1b9fef64828964/kube_codegen.sh#L203-L208

Earlier in the generate-internal-groups.sh script, it was considering all the inputs passed

  echo "Generating conversions"
  "${gobin}/conversion-gen" \
      --input-dirs "$(codegen::join , "${ALL_FQ_APIS[@]}")" \
      -O zz_generated.conversion \
      "$@"

Why is this needed?

--extra-peer-dirs flag is supported by conversion-gen under the hood, but the kube::codegen::gen_helpers function in kube-codegen.sh script is not allowing the flag.

The text was updated successfully, but these errors were encountered:

ary1992 · 2023-09-21T12:12:22Z

/cc @thockin

neolit123 · 2023-09-21T12:43:38Z

/sig api-machinery

jiahuif · 2023-09-21T16:39:47Z

/assign @apelisse
for code generation
/triage accepted

apelisse · 2023-09-21T18:52:43Z

/assign @thockin

thockin · 2023-09-23T17:55:06Z

Can you explain why you need this flag? What I found in k/k was that we don't actually need it at all.

thockin · 2023-09-23T18:00:38Z

You know what, my memory was faulty. We do use it (until I fix that, which is in a development branch, not merged). But we don't use it in any of the "submodule" configs.

thockin · 2023-09-23T18:25:18Z

A lot of the "magic" behind the conversion logic is very brittle. Can you help me understand where / how this impacts you?

thockin · 2023-09-23T18:37:26Z

I whipped up a quick patch but have not tested much:

diff --git a/staging/src/k8s.io/code-generator/kube_codegen.sh b/staging/src/k8s.io/code-generator/kube_codegen.sh
index 3342b9dcaeb..132dea936ab 100755
--- a/staging/src/k8s.io/code-generator/kube_codegen.sh
+++ b/staging/src/k8s.io/code-generator/kube_codegen.sh
@@ -50,11 +50,16 @@ function kube::codegen::internal::git_grep() {
 #   --boilerplate <string = path_to_kube_codegen_boilerplate>
 #     An optional override for the header file to insert into generated files.
 #
+#   --extra-peer-dir <string>
+#     An optional list (this flag may be specified multiple times) of "extra"
+#     directories to consider during conversion generation.
+#
 function kube::codegen::gen_helpers() {
     local in_pkg_root=""
     local out_base="" # gengo needs the output dir must be $out_base/$out_pkg_root
     local boilerplate="${KUBE_CODEGEN_ROOT}/hack/boilerplate.go.txt"
     local v="${KUBE_VERBOSE:-0}"
+    local extra_peers=()
 
     while [ "$#" -gt 0 ]; do
         case "$1" in
@@ -70,6 +75,10 @@ function kube::codegen::gen_helpers() {
                 boilerplate="$2"
                 shift 2
                 ;;
+            "--extra-peer-dir")
+                extra_peers+=("$2")
+                shift 2
+                ;;
             *)
                 echo "unknown argument: $1" >&2
                 return 1
@@ -196,16 +205,21 @@ function kube::codegen::gen_helpers() {
             ":(glob)${root}"/'**/zz_generated.conversion.go' \
             | xargs -0 rm -f
 
-        local inputs=()
+        local input_args=()
         for arg in "${input_pkgs[@]}"; do
-            inputs+=("--input-dirs" "$arg")
+            input_args+=("--input-dirs" "$arg")
+        done
+        local extra_peer_args=()
+        for arg in "${extra_peers[@]}"; do
+            extra_peer_args+=("--extra-peer-dirs" "$arg")
         done
         "${gobin}/conversion-gen" \
             -v "${v}" \
             -O zz_generated.conversion \
             --go-header-file "${boilerplate}" \
             --output-base "${out_base}" \
-            "${inputs[@]}"
+            "${extra_peer_args[@]}" \
+            "${input_args[@]}"
     fi
 }

ary1992 · 2023-09-27T05:30:21Z

A lot of the "magic" behind the conversion logic is very brittle. Can you help me understand where / how this impacts you?

We are using the flag several places in our update-codegen.sh script https://github.com/gardener/gardener/blob/d229a82dacf16808054ab380c2631a007ba4fc5d/hack/update-codegen.sh#L299C2-L306C54

On removal of the flag, the autoConvert functions in. generated.conversion.go files has the compileErrorOnMissingConversion() error :

func autoConvert_v1alpha1_AdmissionControllerConfiguration_To_config_AdmissionControllerConfiguration(in *AdmissionControllerConfiguration, out *config.AdmissionControllerConfiguration, s conversion.Scope) error {
	// FIXME: Provide conversion function to convert configv1alpha1.ClientConnectionConfiguration to componentbaseconfig.ClientConnectionConfiguration
	compileErrorOnMissingConversion()
	out.LogLevel = in.LogLevel
	out.LogFormat = in.LogFormat
	if err := Convert_v1alpha1_ServerConfiguration_To_config_ServerConfiguration(&in.Server, &out.Server, s); err != nil {
		return err
	}
	if in.Debugging != nil {
		in, out := &in.Debugging, &out.Debugging
		*out = new(componentbaseconfig.DebuggingConfiguration)
		// FIXME: Provide conversion function to convert configv1alpha1.DebuggingConfiguration to componentbaseconfig.DebuggingConfiguration
		compileErrorOnMissingConversion()
	} else {
		out.Debugging = nil
	}
	return nil
}

Althoug I can see the function Convert_v1alpha1_ClientConnectionConfiguration_To_config_ClientConnectionConfiguration is already present at vendor/k8s.io/component-base/config/v1alpha1/conversion.go.
But there is also a comment like

// Important! The public back-and-forth conversion functions for the types in this generic
// package with ComponentConfig types need to be manually exposed like this in order for
// other packages that reference this package to be able to call these conversion functions
// in an autogenerated manner.
// TODO: Fix the bug in conversion-gen so it automatically discovers these Convert_* functions
// in autogenerated code as well.

Hence, I think there is a need for the --extra-peer-dirs in order to use this pkg.

thockin · 2023-09-27T19:21:41Z

I see. The component-base is something I didn't account for.

If you apply the above patch, does the new --extra-peer-dir argument to kube_codegen.sh work for you?

ary1992 · 2023-09-28T08:26:29Z

I see. The component-base is something I didn't account for.

If you apply the above patch, does the new --extra-peer-dir argument to kube_codegen.sh work for you?

Above patch throws an error extra_peers[@]: unbound variable for the cases when --extra-peer-dirs flag is not passed to kube::codegen::gen_helpers() function.

Instead I followed this approach :

diff --git a/staging/src/k8s.io/code-generator/kube_codegen.sh b/staging/src/k8s.io/code-generator/kube_codegen.sh
index 3342b9dcaeb..c7d20da0724 100755
--- a/staging/src/k8s.io/code-generator/kube_codegen.sh
+++ b/staging/src/k8s.io/code-generator/kube_codegen.sh
@@ -53,6 +53,7 @@ function kube::codegen::internal::git_grep() {
 function kube::codegen::gen_helpers() {
     local in_pkg_root=""
     local out_base="" # gengo needs the output dir must be $out_base/$out_pkg_root
+    local extra_peers=""
     local boilerplate="${KUBE_CODEGEN_ROOT}/hack/boilerplate.go.txt"
     local v="${KUBE_VERBOSE:-0}"
 
@@ -70,6 +71,10 @@ function kube::codegen::gen_helpers() {
                 boilerplate="$2"
                 shift 2
                 ;;
+            "--extra-peer-dirs")
+                extra_peers+="$2"
+                shift 2
+                ;;
             *)
                 echo "unknown argument: $1" >&2
                 return 1
@@ -205,6 +210,7 @@ function kube::codegen::gen_helpers() {
             -O zz_generated.conversion \
             --go-header-file "${boilerplate}" \
             --output-base "${out_base}" \
+            --extra-peer-dirs "${extra_peers}" \
             "${inputs[@]}"
     fi
 }

thockin · 2023-09-28T15:26:20Z

I'd rather not accumulate into a string - can you tell me what error it throws?

Note, mine wants --extra-peer-dir foo --extra-peer-dir bar

Also - is the argument here a dir (e.g. /home/me/src/foo/bar) or a pkg (e.g. k8s.io/foo/bar) ? I think it's actually a pkg, right?

ary1992 · 2023-10-03T11:13:03Z

I'd rather not accumulate into a string - can you tell me what error it throws?

Note, mine wants --extra-peer-dir foo --extra-peer-dir bar

Also - is the argument here a dir (e.g. /home/me/src/foo/bar) or a pkg (e.g. k8s.io/foo/bar) ? I think it's actually a pkg, right?

You implementation throws error for the cases when we don't specify the --extra-peer-dir option while calling kube::codegen::gen_helpers, as shown below:

  kube::codegen::gen_helpers \
    --input-pkg-root github.com/gardener/gardener/pkg/apis/core \
    --output-base "$(dirname "${BASH_SOURCE[0]}")/../../../.." \
    --boilerplate "${PROJECT_ROOT}/hack/LICENSE_BOILERPLATE.txt"

For such cases I am getting following error

extra_peers[@]: unbound variable

thockin · 2023-10-03T19:18:49Z

It seems to be an old-bash problem.

Try this one?

diff --git a/staging/src/k8s.io/code-generator/kube_codegen.sh b/staging/src/k8s.io/code-generator/kube_codegen.sh
index 3342b9dcaeb..6ded2048368 100755
--- a/staging/src/k8s.io/code-generator/kube_codegen.sh
+++ b/staging/src/k8s.io/code-generator/kube_codegen.sh
@@ -50,11 +50,16 @@ function kube::codegen::internal::git_grep() {
 #   --boilerplate <string = path_to_kube_codegen_boilerplate>
 #     An optional override for the header file to insert into generated files.
 #
+#   --extra-peer-dir <string>
+#     An optional list (this flag may be specified multiple times) of "extra"
+#     directories to consider during conversion generation.
+#
 function kube::codegen::gen_helpers() {
     local in_pkg_root=""
     local out_base="" # gengo needs the output dir must be $out_base/$out_pkg_root
     local boilerplate="${KUBE_CODEGEN_ROOT}/hack/boilerplate.go.txt"
     local v="${KUBE_VERBOSE:-0}"
+    local extra_peers=()
 
     while [ "$#" -gt 0 ]; do
         case "$1" in
@@ -70,6 +75,10 @@ function kube::codegen::gen_helpers() {
                 boilerplate="$2"
                 shift 2
                 ;;
+            "--extra-peer-dir")
+                extra_peers+=("$2")
+                shift 2
+                ;;
             *)
                 echo "unknown argument: $1" >&2
                 return 1
@@ -128,16 +137,16 @@ function kube::codegen::gen_helpers() {
             ":(glob)${root}"/'**/zz_generated.deepcopy.go' \
             | xargs -0 rm -f
 
-        local inputs=()
+        local input_args=()
         for arg in "${input_pkgs[@]}"; do
-            inputs+=("--input-dirs" "$arg")
+            input_args+=("--input-dirs" "$arg")
         done
         "${gobin}/deepcopy-gen" \
             -v "${v}" \
             -O zz_generated.deepcopy \
             --go-header-file "${boilerplate}" \
             --output-base "${out_base}" \
-            "${inputs[@]}"
+            "${input_args[@]}"
     fi
 
     # Defaults
@@ -162,16 +171,16 @@ function kube::codegen::gen_helpers() {
             ":(glob)${root}"/'**/zz_generated.defaults.go' \
             | xargs -0 rm -f
 
-        local inputs=()
+        local input_args=()
         for arg in "${input_pkgs[@]}"; do
-            inputs+=("--input-dirs" "$arg")
+            input_args+=("--input-dirs" "$arg")
         done
         "${gobin}/defaulter-gen" \
             -v "${v}" \
             -O zz_generated.defaults \
             --go-header-file "${boilerplate}" \
             --output-base "${out_base}" \
-            "${inputs[@]}"
+            "${input_args[@]}"
     fi
 
     # Conversions
@@ -196,16 +205,21 @@ function kube::codegen::gen_helpers() {
             ":(glob)${root}"/'**/zz_generated.conversion.go' \
             | xargs -0 rm -f
 
-        local inputs=()
+        local input_args=()
         for arg in "${input_pkgs[@]}"; do
-            inputs+=("--input-dirs" "$arg")
+            input_args+=("--input-dirs" "$arg")
+        done
+        local extra_peer_args=()
+        for arg in "${extra_peers[@]:+"${extra_peers[@]}"}"; do
+            extra_peer_args+=("--extra-peer-dirs" "$arg")
         done
         "${gobin}/conversion-gen" \
             -v "${v}" \
             -O zz_generated.conversion \
             --go-header-file "${boilerplate}" \
             --output-base "${out_base}" \
-            "${inputs[@]}"
+            "${extra_peer_args[@]:+"${extra_peer_args[@]}"}" \
+            "${input_args[@]}"
     fi
 }

ary1992 · 2023-10-04T09:08:45Z

Try this one?

Yes this is working fine.

thockin · 2023-10-04T17:08:32Z

OK. I'll prep a PR soon.

ary1992 · 2023-10-04T17:39:46Z

OK. I'll prep a PR soon.

Thank you

ary1992 added the kind/feature Categorizes issue or PR as related to a new feature. label Sep 21, 2023

k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 21, 2023

k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 21, 2023

k8s-ci-robot assigned apelisse Sep 21, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 21, 2023

k8s-ci-robot assigned thockin Sep 21, 2023

ary1992 mentioned this issue Sep 25, 2023

Upgrade k8s.io/* to v0.28, sigs.k8s.io/controller-runtime to v0.16 gardener/gardener#8382

Closed

2 tasks

thockin mentioned this issue Oct 4, 2023

Add --extra-peer in conversion in kube_codegen #120994

Merged

k8s-ci-robot closed this as completed in #120994 Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Code-generator] `--extra-peer-dirs` flag is not supported by new `kube-codegen.sh` script #120803

[Code-generator] `--extra-peer-dirs` flag is not supported by new `kube-codegen.sh` script #120803

ary1992 commented Sep 21, 2023 •

edited

ary1992 commented Sep 21, 2023

neolit123 commented Sep 21, 2023

jiahuif commented Sep 21, 2023

apelisse commented Sep 21, 2023

thockin commented Sep 23, 2023

thockin commented Sep 23, 2023 •

edited

thockin commented Sep 23, 2023

thockin commented Sep 23, 2023 •

edited

ary1992 commented Sep 27, 2023 •

edited

thockin commented Sep 27, 2023

ary1992 commented Sep 28, 2023 •

edited

thockin commented Sep 28, 2023

ary1992 commented Oct 3, 2023

thockin commented Oct 3, 2023

ary1992 commented Oct 4, 2023

thockin commented Oct 4, 2023

ary1992 commented Oct 4, 2023

[Code-generator] --extra-peer-dirs flag is not supported by new kube-codegen.sh script #120803

[Code-generator] --extra-peer-dirs flag is not supported by new kube-codegen.sh script #120803

Comments

ary1992 commented Sep 21, 2023 • edited

What would you like to be added?

Why is this needed?

ary1992 commented Sep 21, 2023

neolit123 commented Sep 21, 2023

jiahuif commented Sep 21, 2023

apelisse commented Sep 21, 2023

thockin commented Sep 23, 2023

thockin commented Sep 23, 2023 • edited

thockin commented Sep 23, 2023

thockin commented Sep 23, 2023 • edited

ary1992 commented Sep 27, 2023 • edited

thockin commented Sep 27, 2023

ary1992 commented Sep 28, 2023 • edited

thockin commented Sep 28, 2023

ary1992 commented Oct 3, 2023

thockin commented Oct 3, 2023

ary1992 commented Oct 4, 2023

thockin commented Oct 4, 2023

ary1992 commented Oct 4, 2023

[Code-generator] `--extra-peer-dirs` flag is not supported by new `kube-codegen.sh` script #120803

[Code-generator] `--extra-peer-dirs` flag is not supported by new `kube-codegen.sh` script #120803

ary1992 commented Sep 21, 2023 •

edited

thockin commented Sep 23, 2023 •

edited

thockin commented Sep 23, 2023 •

edited

ary1992 commented Sep 27, 2023 •

edited

ary1992 commented Sep 28, 2023 •

edited