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

conversion-gen: cut off kube dependencies in extra-peer-dirs #54394

Conversation

sttts
Copy link
Contributor

@sttts sttts commented Oct 23, 2017

Fixes #54301

This makes conversion-gen usable in a context without a vendored k8s.io/kubernetes.

In conversion-gen removed Kubernetes core API from default extra-peer-dirs.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 23, 2017
@sttts sttts assigned munnerz and unassigned cblecker and spxtr Oct 23, 2017
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 23, 2017
@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Oct 23, 2017
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Oct 23, 2017
@munnerz
Copy link
Member

munnerz commented Oct 23, 2017

👍 this looks good to me. I guess being able to specify custom values for the conversion generator is something that can be followed up, but this sorts out the issues we're having with the generate-* scripts in 3rd party projects that don't want to vendor k8s.io/kubernetes.

How does this affect the existing uses of these generate-* scripts in k8s.io/sample-apiserver et al? Does the sample (or similar) depend on anything in k8s.io/kubernetes/pkg/api/... etc? I'd guess not 😄

@munnerz
Copy link
Member

munnerz commented Oct 23, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2017
@sttts
Copy link
Contributor Author

sttts commented Oct 23, 2017

How does this affect the existing uses of these generate-* scripts in k8s.io/sample-apiserver et al? Does the sample (or similar) depend on anything in k8s.io/kubernetes/pkg/api/... etc? I'd guess not

The types are simple enough. We used to have a couple of shared types in core/v1, but everything should have moved to metav1. Would be interesting to see whether our conversion change at all if I remove the lines from Makefile.generated_files.

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Oct 23, 2017
@sttts
Copy link
Contributor Author

sttts commented Oct 23, 2017

Added another commit to remove core/v1 peer dirs, removed the conversion files and regenerated them, without any difference. So it looks like we don't need them anymore.

I know at least from OpenShift that the peer dirs are used extensively there. Maybe we could switch to tage as well though.

@@ -51,14 +51,6 @@ func main() {
// TODO: make callers pass this in. It is too opaque here, and any use of
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A similar TODO should follow the code that inserts magic - can you put a comment in the new place and strike this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have split the peer dirs into base-peer-dirs (apimachinery only) and extra-peer-dirs. The former are basically constant for everybody. The later are api specific, e.g. Kubernetes. For the very rare situations that you want to modify the base-peer-dirs as well, you can. But we can maintain that list without breaking people using extra-peer-dirs.

@@ -245,11 +245,14 @@ func Packages(context *generator.Context, arguments *args.GeneratorArgs) generat
glog.V(5).Infof(" no tag")
continue
}
peerPkgs = append(peerPkgs,
"k8s.io/apimachinery/pkg/apis/meta/v1",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't the removal of the core APIs break some of the generated conversions? We need to make sure these are provided by the Makefile:

$ vi staging/src/k8s.io/code-generator/cmd/conversion-gen/main.go 

$ make generated_files 
+++ [1025 16:43:10] Building the toolchain targets:
    k8s.io/kubernetes/hack/cmd/teststale
    k8s.io/kubernetes/vendor/github.com/jteeuwen/go-bindata/go-bindata
+++ [1025 16:43:10] Generating bindata:
    test/e2e/generated/gobindata_util.go
~/src/go/src/k8s.io/kubernetes ~/src/go/src/k8s.io/kubernetes/test/e2e/generated
~/src/go/src/k8s.io/kubernetes/test/e2e/generated
+++ [1025 16:43:10] Building go targets for linux/amd64:
    ./vendor/k8s.io/code-generator/cmd/conversion-gen

$ rm pkg/apis/apps/v1/zz_generated.conversion.go

$ make generated_files 

$ git diff --text pkg/apis/apps/v1/zz_generated.conversion.go
diff --git a/pkg/apis/apps/v1/zz_generated.conversion.go b/pkg/apis/apps/v1/zz_generated.conversion.go
index a93ad57d0f..d32e45e313 100644
--- a/pkg/apis/apps/v1/zz_generated.conversion.go
+++ b/pkg/apis/apps/v1/zz_generated.conversion.go
@@ -25,7 +25,6 @@ import (
        meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
        conversion "k8s.io/apimachinery/pkg/conversion"
        runtime "k8s.io/apimachinery/pkg/runtime"
-       api_v1 "k8s.io/kubernetes/pkg/api/v1"
        extensions "k8s.io/kubernetes/pkg/apis/extensions"
        unsafe "unsafe"
 )
@@ -119,7 +118,8 @@ func Convert_extensions_DaemonSetList_To_v1_DaemonSetList(in *extensions.DaemonS
 
 func autoConvert_v1_DaemonSetSpec_To_extensions_DaemonSetSpec(in *v1.DaemonSetSpec, out *extensions.DaemonSetSpec, s conversion.Scope) error {
        out.Selector = (*meta_v1.LabelSelector)(unsafe.Pointer(in.Selector))
-       if err := api_v1.Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+       // TODO: Inefficient conversion - can we improve it?
+       if err := s.Convert(&in.Template, &out.Template, 0); err != nil {
                return err
        }
        if err := Convert_v1_DaemonSetUpdateStrategy_To_extensions_DaemonSetUpdateStrategy(&in.UpdateStrategy, &out.UpdateStrategy, s); err != nil {
...

Normally those HAPPEN to be in the list, but if ever they are not -- kaboom. So this PR needs to include them in Makefile.generated_files ~L640, and if we do that we should just do these 3 also?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Double checked again and regenerated all conversion, without any modification. Looks like we have covered everything with tags.

I can drop the second commits if you like, just to be on the safe side. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was that it works if you regenerate them ALL but it may not work if you only regenerate some.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, if you walk through my script, it is still broken. We either need to always generate all (in which case some of the makefile can be simplified) or we need to make sure that the common deps are always in the peer list. You can repro this:

thockin@freakshow2 kubernetes master /$ git copr 54394
remote: Counting objects: 4217, done.
remote: Compressing objects: 100% (281/281), done.
remote: Total 4217 (delta 2734), reused 2797 (delta 2630), pack-reused 1276
Receiving objects: 100% (4217/4217), 9.13 MiB | 22.97 MiB/s, done.
Resolving deltas: 100% (3169/3169), completed with 1451 local objects.
From https://github.com/kubernetes/kubernetes
 * [new ref]               refs/pull/54394/head -> pr-54394
 * [new tag]               v1.9.0-alpha.2       -> v1.9.0-alpha.2
Switched to branch 'pr-54394'

thockin@freakshow2 kubernetes pr-54394 /$ make generated_files 
+++ [1102 15:29:47] Building the toolchain targets:
    k8s.io/kubernetes/hack/cmd/teststale
    k8s.io/kubernetes/vendor/github.com/jteeuwen/go-bindata/go-bindata
+++ [1102 15:29:47] Generating bindata:
    test/e2e/generated/gobindata_util.go
~/src/go/src/k8s.io/kubernetes ~/src/go/src/k8s.io/kubernetes/test/e2e/generated
~/src/go/src/k8s.io/kubernetes/test/e2e/generated
+++ [1102 15:29:49] Building go targets for linux/amd64:
    ./vendor/k8s.io/code-generator/cmd/conversion-gen
+++ [1102 15:30:08] Building the toolchain targets:
    k8s.io/kubernetes/hack/cmd/teststale
    k8s.io/kubernetes/vendor/github.com/jteeuwen/go-bindata/go-bindata
+++ [1102 15:30:08] Generating bindata:
    test/e2e/generated/gobindata_util.go
~/src/go/src/k8s.io/kubernetes ~/src/go/src/k8s.io/kubernetes/test/e2e/generated
~/src/go/src/k8s.io/kubernetes/test/e2e/generated
+++ [1102 15:30:10] Building go targets for linux/amd64:
    ./vendor/k8s.io/code-generator/cmd/openapi-gen

thockin@freakshow2 kubernetes pr-54394 /$ git st | grep zz_gen
thockin@freakshow2 kubernetes pr-54394 /$

thockin@freakshow2 kubernetes pr-54394 /$ rm pkg/apis/apps/v1/zz_generated.conversion.go

thockin@freakshow2 kubernetes pr-54394 /$ make generated_files 

thockin@freakshow2 kubernetes pr-54394 /$ git diff --text
diff --git a/pkg/apis/apps/v1/zz_generated.conversion.go b/pkg/apis/apps/v1/zz_generated.conversion.go
index a93ad57d0f..d32e45e313 100644
--- a/pkg/apis/apps/v1/zz_generated.conversion.go
+++ b/pkg/apis/apps/v1/zz_generated.conversion.go
@@ -25,7 +25,6 @@ import (
        meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
        conversion "k8s.io/apimachinery/pkg/conversion"
        runtime "k8s.io/apimachinery/pkg/runtime"
-       api_v1 "k8s.io/kubernetes/pkg/api/v1"
        extensions "k8s.io/kubernetes/pkg/apis/extensions"
        unsafe "unsafe"
 )
@@ -119,7 +118,8 @@ func Convert_extensions_DaemonSetList_To_v1_DaemonSetList(in *extensions.DaemonS
 
 func autoConvert_v1_DaemonSetSpec_To_extensions_DaemonSetSpec(in *v1.DaemonSetSpec, out *extensions.DaemonSetSpec, s conversion.Scope) error {
        out.Selector = (*meta_v1.LabelSelector)(unsafe.Pointer(in.Selector))
-       if err := api_v1.Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+       // TODO: Inefficient conversion - can we improve it?
+       if err := s.Convert(&in.Template, &out.Template, 0); err != nil {
                return err
        }
        if err := Convert_v1_DaemonSetUpdateStrategy_To_extensions_DaemonSetUpdateStrategy(&in.UpdateStrategy, &out.UpdateStrategy, s); err != nil {
@@ -132,7 +132,8 @@ func autoConvert_v1_DaemonSetSpec_To_extensions_DaemonSetSpec(in *v1.DaemonSetSp
 
 func autoConvert_extensions_DaemonSetSpec_To_v1_DaemonSetSpec(in *extensions.DaemonSetSpec, out *v1.DaemonSetSpec, s conversion.Scope) error {
        out.Selector = (*meta_v1.LabelSelector)(unsafe.Pointer(in.Selector))
-       if err := api_v1.Convert_api_PodTemplateSpec_To_v1_PodTemplateSpec(&in.Template, &out.Template, s); err != nil {
+       // TODO: Inefficient conversion - can we improve it?
+       if err := s.Convert(&in.Template, &out.Template, 0); err != nil {
                return err

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, managed to reproduce now. Removed the second commit.

@sttts
Copy link
Contributor Author

sttts commented Oct 26, 2017

Won't the removal of the core APIs break some of the generated conversions? We need to make sure these are provided by the Makefile:

Compare my two commits. The first one just moves them into the Makefile. The second one removes them after I noticed during regeneration that they don't matter anymore.

@sttts sttts force-pushed the sttts-conversion-gen-kube-peer-dirs branch from 2437538 to 261e9c3 Compare November 2, 2017 16:10
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Nov 2, 2017
@sttts sttts force-pushed the sttts-conversion-gen-kube-peer-dirs branch from 261e9c3 to d1e0a9d Compare November 3, 2017 08:13
@k8s-github-robot k8s-github-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2017
@thockin
Copy link
Member

thockin commented Nov 3, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: munnerz, sttts, thockin

Associated issue: 54301

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51874, 54394). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 7ec4790 into kubernetes:master Nov 3, 2017
k8s-github-robot pushed a commit that referenced this pull request Nov 18, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following #54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```
sttts pushed a commit to sttts/code-generator that referenced this pull request Nov 19, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
sttts pushed a commit to sttts/code-generator that referenced this pull request Nov 27, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
sttts pushed a commit to sttts/code-generator that referenced this pull request Nov 28, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
sttts pushed a commit to sttts/code-generator that referenced this pull request Nov 28, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
sttts pushed a commit to sttts/code-generator that referenced this pull request Nov 28, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
sttts pushed a commit to sttts/code-generator that referenced this pull request Nov 28, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
k8s-publishing-bot pushed a commit to k8s-publishing-bot/code-generator that referenced this pull request Nov 29, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
k8s-publishing-bot pushed a commit to k8s-publishing-bot/code-generator that referenced this pull request Dec 7, 2017
Automatic merge from submit-queue (batch tested with PRs 55841, 55948, 55945). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

conversion-gen: check for nil pkg in getManualConversionFunctions

**What this PR does / why we need it**:

Following kubernetes/kubernetes#54394, this PR performs a check in getManualConversionFunctions for packages that are not found and prints a warning instead of panicking.

This should improve the UX of conversion-gen, and is also a narrower change than that proposed in #54394, so hopefully can be cherry picked into the release-1.8 etc. branches.

This would allow users to use conversion-gen via the generate-internal-groups.sh script without having to make a second call that 'blanks out' `--extra-peer-dirs`.

/cc @sttts

```release-note
NONE
```

Kubernetes-commit: a83f78efc9f98b71b4604acf6ba14a4a51eceac8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants