Don't check in zz_generated.openapi.go. #41106

spxtr · 2017-02-08T00:00:12Z

zz_generated.openapi.go is the file that causes the most merge conflicts of all. In #33440, @thockin updated the makefile to support generating these files on demand, but that didn't play well with bazel/gazel.

In this PR, I add a new build macro that will generate this file with a go_genrule. I added support for keeping the BUILD file up to date in mikedanese/gazel#34.

Release note:

NONE

k8s-reviewable · 2017-02-08T00:01:18Z

This change is

mikedanese · 2017-02-09T22:12:05Z

hack/update-bazel.sh

@@ -22,5 +22,8 @@ source "${KUBE_ROOT}/hack/lib/init.sh"

 git config http.https://gopkg.in.followRedirects true

-go get -u gopkg.in/mikedanese/gazel.v13/gazel
+# Remove generated files prior to running gazel.
+rm -f "${KUBE_ROOT}/pkg/generated/openapi/zz_generated.openapi.go"


Eventually this will be make clean_generated instead, but for now this will have to do.

gazel isn't smart enough to do it any other way iiuc.

thockin · 2017-02-13T21:51:50Z

hack/update-bazel.sh

@@ -22,5 +22,8 @@ source "${KUBE_ROOT}/hack/lib/init.sh"

 git config http.https://gopkg.in.followRedirects true

+# Remove generated files prior to running gazel.
+rm -f "${KUBE_ROOT}/pkg/generated/openapi/zz_generated.openapi.go"


Will this exist forever? This means that any time we run gazel, we will have to rebuild all generated files, even if net nothing changed...

Or does this come with a TODO?

If we only use Bazel to build then we can delete this, since Bazel doesn't put the generated file in with the rest of the source. As long as we support both Bazel and make we'll need this.

We could consider adding the ability to gazel to move files to somewhere else and then back after the run, but I don't think it's worth it.

Can we maybe use +build tags to make bazel ignore it or something? Or can it come with a TODO comment to remove this line completely once Bazel is the only way to build?

Added a TODO. Our generated code has the !ignore_autogenerated tag on it that we might be able to use, but only once bazel can generate all of our code.

thockin · 2017-02-13T21:53:47Z

hack/verify-bazel-generated.sh

+export KUBE_ROOT=$(dirname "${BASH_SOURCE}")/..
+cd "${KUBE_ROOT}"
+# List go packages with the k8s:openapi-gen tag.
+files=$(grep --color=never -l '+k8s:openapi-gen=' \


This is going to include vendor/ - is that intended? The find used in the Make process is very carefully written...

I copied the find from the makefile, but I'm not sure that there's a good way to keep it up to date. We do want to include vendor/.

Using the make process, if I touch vendor/k8s.io/apimachinery/pkg/apis/meta/v1/doc.go then run make generated_files, it will recreate the generated files under there. Evidently the source of truth for that stuff is vendor/...

ugggggggggh, I didn't see that coming - the repo splits now force us to codegen for a vendored repo.

but when we codegen, we exclude vendor...

hack/make-rules/helpers/cache_go_dirs.sh

how does it work?

If I rm vendor/k8s.io/apimachinery/pkg/apis/meta/v1/zz_generated.deepcopy.go and then run make generated_files, it recreates the file. Not sure how it works, but for now I was planning on maintaining that behavior.

This whole staging and vendor thing is super confusing.

thockin · 2017-02-13T21:54:16Z

hack/verify-bazel.sh

@@ -22,6 +22,9 @@ source "${KUBE_ROOT}/hack/lib/init.sh"

 git config http.https://gopkg.in.followRedirects true

+# Remove generated files prior to running gazel.
+rm -f "${KUBE_ROOT}/pkg/generated/openapi/zz_generated.openapi.go"


same comment as above

Same response.

same response

Copied the TODO here as well.

thockin · 2017-02-13T21:57:01Z

pkg/generated/openapi/def.bzl

+
+
+def openapi_library(name, tags, srcs):
+  deps = [


is this file generated? From what? I'm not seeing where it originates..

No, this file is hand-written. However, gazel will still automanage the go_default_library in pkg/generated/openapi/BUILD. This isn't a big deal for this generated file, since its only source is doc.go, but it will be important for the other zz_generated files.

spxtr · 2017-02-14T01:41:19Z

Trivial rebase, no changes outside of zz_generated.openapi.go, which is deleted.

spxtr · 2017-02-16T23:12:42Z

@thockin PTAL

thockin · 2017-02-17T07:07:12Z

This seems OK (modulo comments) for this, but now I am worried about the other generators wrt vendor/ - should we hold off on merge until we know we know what is happening there?

spxtr · 2017-02-17T19:42:10Z

@mikedanese has a PR out (#40777) to put BUILD files in individual vendor directories. This will significantly cut back on rebases, since vendor/BUILD is terrible, but it will also make it so that I can autogenerate the files in vendor/k8s.io/apimachinery and the like using the same strategy.

I think this PR can go in before we know what happens with vendor/ and staging/, but the next one that will add zz_generated.{deepcopy,conversion,defaults}.go should probably wait.

spxtr · 2017-02-22T18:14:07Z

pkg/generated/openapi/def.bzl

+load("@io_kubernetes_build//defs:go.bzl", "go_genrule")
+
+OPENAPI_TARGETS = [
+    "cmd/libs/go2idl/client-gen/test_apis/testgroup/v1",


@thockin: After talking with @mikedanese, I think it's a good idea to automanage these lists with gazel.

Clean zz_generated.openapi.go before running gazel.

spxtr · 2017-04-26T00:52:38Z

#40777 is in, so this can proceed.

thockin · 2017-04-26T04:42:02Z

pkg/generated/openapi/BUILD

-        "doc.go",
-        "zz_generated.openapi.go",
+    srcs = ["doc.go"],
+    openapi_targets = [


Remind me - this list is manually maintained? Or do we auto-figure that out?

Gazel automanages it :)

thockin · 2017-04-26T04:44:09Z

I'm OK to get this in, but I think being manually managed will catch up with us very quickly.

spxtr · 2017-04-26T06:26:03Z

@k8s-bot gce etcd3 e2e test this

thockin · 2017-04-26T06:32:04Z

I updated #33440

…

On Tue, Apr 25, 2017 at 11:26 PM, Joe Finney ***@***.***> wrote: @k8s-bot <https://github.com/k8s-bot> gce etcd3 e2e test this — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41106 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVHjKqCuQNB-IyYRt4Qewvdif2-Nbks5rzuONgaJpZM4L6NGf> .

spxtr · 2017-04-26T20:02:24Z

Do I need to change anything in this one or should it go in as-is? It's going to make #3340 need a rebase.

thockin · 2017-04-27T00:31:30Z

Do we want to flatten all of these commits into a single PR, in case of rollback? If not, let's go for it

…

On Wed, Apr 26, 2017 at 1:02 PM, Joe Finney ***@***.***> wrote: Do I need to change anything in this one or should it go in as-is? It's going to make #3340 <#3340> need a rebase. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41106 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFVgVA6rCfvOi7WdwnRRQF6Uvky9Cqzwks5rz6LjgaJpZM4L6NGf> .

spxtr · 2017-04-27T00:54:19Z

If we end up merging both and need to rollback both then we can do so in one PR with multiple git revert calls.

mikedanese · 2017-04-27T15:31:42Z

I'm happy to give this a shot when you guys are.

multiple git revert calls.

git revert allows for a revert of a merge commit, which reverts all commits that were merged in a single go. Checkout git revert -m option.

mikedanese · 2017-04-27T15:35:20Z

/lgtm

thockin · 2017-04-27T16:04:54Z

/approve

k8s-github-robot · 2017-04-27T16:06:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikedanese, spxtr, thockin

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

~~OWNERS~~ [thockin]

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

thockin

My PR still needs genrules :)

thockin · 2017-04-27T16:06:25Z

hack/update-bazel.sh

-go get gopkg.in/mikedanese/gazel.v16/gazel
+# Remove generated files prior to running gazel.
+# TODO(spxtr): Remove this line once Bazel is the only way to build.
+rm -f "${KUBE_ROOT}/pkg/generated/openapi/zz_generated.openapi.go"


In my PR I went the other way - force it to be generated before running gazel. Otherwise some symbols are missing for other genfiles

k8s-github-robot · 2017-04-27T18:40:31Z

Automatic merge from submit-queue

spxtr assigned thockin Feb 8, 2017

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 8, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels Feb 8, 2017

spxtr force-pushed the gen3 branch from b0e9b8d to a04f6b9 Compare February 8, 2017 00:18

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-label-needed labels Feb 8, 2017

spxtr force-pushed the gen3 branch 3 times, most recently from 421831e to 16a502a Compare February 8, 2017 01:49

spxtr assigned mikedanese Feb 8, 2017

spxtr mentioned this pull request Feb 9, 2017

bump gazel to v14 #41210

Merged

mikedanese reviewed Feb 9, 2017

View reviewed changes

spxtr force-pushed the gen3 branch 2 times, most recently from a7c8780 to 8626cdc Compare February 10, 2017 19:46

thockin reviewed Feb 13, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

spxtr force-pushed the gen3 branch from 8626cdc to e18444e Compare February 14, 2017 01:40

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 14, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 16, 2017

spxtr force-pushed the gen3 branch from ea24077 to 78b37e2 Compare February 17, 2017 19:51

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 17, 2017

spxtr commented Feb 22, 2017

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 30, 2017

spxtr added 3 commits April 25, 2017 17:41

Remove zz_generated.openapi.go.

48fc03b

Remove verify_gen_openapi make rule.

3bc3e41

Add go_genrule for zz_generated.openapi.go.

8e84a9a

Clean zz_generated.openapi.go before running gazel.

spxtr force-pushed the gen3 branch from 00b5712 to 8e84a9a Compare April 26, 2017 00:51

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 26, 2017

spxtr assigned ixdy Apr 26, 2017

thockin reviewed Apr 26, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 27, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 27, 2017

thockin reviewed Apr 27, 2017

View reviewed changes

mikedanese added do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Apr 27, 2017

spxtr mentioned this pull request Apr 27, 2017

Don't commit generated (some) code #33440

Closed

k8s-github-robot merged commit 98398d5 into kubernetes:master Apr 27, 2017

spxtr deleted the gen3 branch April 27, 2017 18:49

ixdy mentioned this pull request Apr 27, 2018

Don't check in generated code #8830

Closed

roycaihw mentioned this pull request Sep 11, 2019

Commit generated openapi definitions #81239

Closed

Don't check in zz_generated.openapi.go. #41106

Don't check in zz_generated.openapi.go. #41106

Conversation

spxtr commented Feb 8, 2017 • edited Loading

k8s-reviewable commented Feb 8, 2017

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

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

spxtr commented Feb 14, 2017

spxtr commented Feb 16, 2017

thockin commented Feb 17, 2017

spxtr commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spxtr commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thockin commented Apr 26, 2017

spxtr commented Apr 26, 2017

thockin commented Apr 26, 2017 via email

spxtr commented Apr 26, 2017

thockin commented Apr 27, 2017 via email

spxtr commented Apr 27, 2017

mikedanese commented Apr 27, 2017

mikedanese commented Apr 27, 2017

thockin commented Apr 27, 2017

k8s-github-robot commented Apr 27, 2017

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-github-robot commented Apr 27, 2017

spxtr commented Feb 8, 2017 •

edited

Loading