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

Move generators to staging/src/k8s.io/kube-gen #49114

Merged
merged 4 commits into from Jul 20, 2017

Conversation

Projects
None yet
8 participants
@sttts
Contributor

sttts commented Jul 18, 2017

Reason number one for people trying the impossible of vendoring kube: reuse of the generators.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 18, 2017

Contributor

/cc @maleck13 ptal

Contributor

sttts commented Jul 18, 2017

/cc @maleck13 ptal

@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Jul 18, 2017

Contributor

@sttts: GitHub didn't allow me to request PR reviews from the following users: maleck13, ptal.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @maleck13 ptal

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Contributor

k8s-ci-robot commented Jul 18, 2017

@sttts: GitHub didn't allow me to request PR reviews from the following users: maleck13, ptal.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @maleck13 ptal

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jul 18, 2017

Contributor

/lgtm

(It won't merge without a top level approver.)

I'm definitely a fan.

Contributor

deads2k commented Jul 18, 2017

/lgtm

(It won't merge without a top level approver.)

I'm definitely a fan.

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 18, 2017

@maleck13

This comment has been minimized.

Show comment
Hide comment
@maleck13

maleck13 Jul 18, 2017

Contributor

@sttts Am i understanding correctly that this would allow us to vendor the generator pkgs in a similar way to the apiserver and apimachinery. If so I love it. Was wondering recently how to keep the versions in sync and it seemed vendoring k8s was the only way.

Contributor

maleck13 commented Jul 18, 2017

@sttts Am i understanding correctly that this would allow us to vendor the generator pkgs in a similar way to the apiserver and apimachinery. If so I love it. Was wondering recently how to keep the versions in sync and it seemed vendoring k8s was the only way.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jul 18, 2017

Contributor

@sttts Am i understanding correctly that this would allow us to vendor the generator pkgs in a similar way to the apiserver and apimachinery. If so I love it. Was wondering recently how to keep the versions in sync and it seemed vendoring k8s was the only way.

Yes, that's the intent here. There is more work to do to make them truly re-usable, but this would be a good first step.

Contributor

deads2k commented Jul 18, 2017

@sttts Am i understanding correctly that this would allow us to vendor the generator pkgs in a similar way to the apiserver and apimachinery. If so I love it. Was wondering recently how to keep the versions in sync and it seemed vendoring k8s was the only way.

Yes, that's the intent here. There is more work to do to make them truly re-usable, but this would be a good first step.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Jul 18, 2017

Member

Is this ready to go? I am happy to approve it

Member

lavalamp commented Jul 18, 2017

Is this ready to go? I am happy to approve it

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 18, 2017

Contributor

@lavalamp looks like some verify errors are left. Will take care of them tomorrow. Feel free to approve it now :)

Contributor

sttts commented Jul 18, 2017

@lavalamp looks like some verify errors are left. Will take care of them tomorrow. Feel free to approve it now :)

@caesarxuchao

This comment has been minimized.

Show comment
Hide comment
@caesarxuchao

caesarxuchao Jul 18, 2017

Member

One question, do we need to set up the publish robot to sync the repo, or are we going to move it its own repo and vendor it back? I think the latter is preferred, since those generators are relatively stable.

Member

caesarxuchao commented Jul 18, 2017

One question, do we need to set up the publish robot to sync the repo, or are we going to move it its own repo and vendor it back? I think the latter is preferred, since those generators are relatively stable.

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jul 18, 2017

Contributor

One question, do we need to set up the publish robot to sync the repo, or are we going to move it its own repo and vendor it back? I think the latter is preferred, since those generators are relatively stable.

I don't feel strongly either way, but I do think we have a significant amount of work to do on them to make them re-useable before we move them out of tree. I suspect they'll be fast moving until we complete the work of making them flag driven instead of hardcoded and I'd want to be certain that they work with kube during that process.

I could even see an argument that says we should never have a level of the kube generators that doesn't generate working code for us and developing them in an authoritative staging repo ensures that.

Contributor

deads2k commented Jul 18, 2017

One question, do we need to set up the publish robot to sync the repo, or are we going to move it its own repo and vendor it back? I think the latter is preferred, since those generators are relatively stable.

I don't feel strongly either way, but I do think we have a significant amount of work to do on them to make them re-useable before we move them out of tree. I suspect they'll be fast moving until we complete the work of making them flag driven instead of hardcoded and I'd want to be certain that they work with kube during that process.

I could even see an argument that says we should never have a level of the kube generators that doesn't generate working code for us and developing them in an authoritative staging repo ensures that.

@lavalamp

This comment has been minimized.

Show comment
Hide comment
@lavalamp

lavalamp Jul 18, 2017

Member

Since this already needs a rebase I went ahead and approved #49055.

I'll approve this ahead of time and then you'll just need an lgtm after you rebase.

/approve

Member

lavalamp commented Jul 18, 2017

Since this already needs a rebase I went ahead and approved #49055.

I'll approve this ahead of time and then you'll just need an lgtm after you rebase.

/approve

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Jul 18, 2017

Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, lavalamp, sttts

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

Contributor

k8s-merge-robot commented Jul 18, 2017

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: deads2k, lavalamp, sttts

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

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

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 19, 2017

Contributor

Fixed the verify issue: client-gen -t also needed an output base to find its test group. Otherwise, nothing serious changed.

Contributor

sttts commented Jul 19, 2017

Fixed the verify issue: client-gen -t also needed an output base to find its test group. Otherwise, nothing serious changed.

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 20, 2017

Contributor

/retest

Contributor

sttts commented Jul 20, 2017

/retest

@sttts sttts added lgtm and removed lgtm needs-rebase labels Jul 20, 2017

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 20, 2017

Contributor

/retest

Contributor

sttts commented Jul 20, 2017

/retest

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 20, 2017

Contributor

/retest

Contributor

sttts commented Jul 20, 2017

/retest

@deads2k

This comment has been minimized.

Show comment
Hide comment
@deads2k

deads2k Jul 20, 2017

Contributor

/retest

Contributor

deads2k commented Jul 20, 2017

/retest

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Jul 20, 2017

Contributor

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

Contributor

k8s-merge-robot commented Jul 20, 2017

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

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 20, 2017

Contributor

/retest

Contributor

sttts commented Jul 20, 2017

/retest

@k8s-merge-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-merge-robot

k8s-merge-robot Jul 20, 2017

Contributor

Automatic merge from submit-queue (batch tested with PRs 49114, 48810)

Contributor

k8s-merge-robot commented Jul 20, 2017

Automatic merge from submit-queue (batch tested with PRs 49114, 48810)

@k8s-merge-robot k8s-merge-robot merged commit c3a9270 into kubernetes:master Jul 20, 2017

8 of 11 checks passed

pull-kubernetes-federation-e2e-gce Jenkins job failed.
Details
Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce-etcd3
Details
pull-kubernetes-e2e-gce-etcd3 Jenkins job triggered.
Details
cla/linuxfoundation sttts authorized
Details
pull-kubernetes-bazel Job succeeded.
Details
pull-kubernetes-cross Jenkins job succeeded.
Details
pull-kubernetes-e2e-kops-aws Jenkins job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Jenkins job succeeded.
Details
pull-kubernetes-node-e2e Jenkins job succeeded.
Details
pull-kubernetes-unit Jenkins job succeeded.
Details
pull-kubernetes-verify Jenkins job succeeded.
Details
@k8s-ci-robot

This comment has been minimized.

Show comment
Hide comment
@k8s-ci-robot

k8s-ci-robot Jul 20, 2017

Contributor

@sttts: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce 660ada4 link /test pull-kubernetes-federation-e2e-gce
pull-kubernetes-e2e-gce-etcd3 660ada4 link /test pull-kubernetes-e2e-gce-etcd3

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Contributor

k8s-ci-robot commented Jul 20, 2017

@sttts: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-federation-e2e-gce 660ada4 link /test pull-kubernetes-federation-e2e-gce
pull-kubernetes-e2e-gce-etcd3 660ada4 link /test pull-kubernetes-e2e-gce-etcd3

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@mbohlool

This comment has been minimized.

Show comment
Hide comment
@mbohlool

mbohlool Jul 20, 2017

Member

Do you plan to keep this in the staging/ forever? As far as I can tell, we can't vendor this in, without some boring hack, if we plan to develop it in its own repo (godep does not allow importing main packages).

Member

mbohlool commented Jul 20, 2017

Do you plan to keep this in the staging/ forever? As far as I can tell, we can't vendor this in, without some boring hack, if we plan to develop it in its own repo (godep does not allow importing main packages).

@sttts

This comment has been minimized.

Show comment
Hide comment
@sttts

sttts Jul 21, 2017

Contributor

if we plan to develop it in its own repo

In the worst case we can manually keep the main package in the vendor tree. It's a godep issue, not a vendor-experiment issue.

I hope we find a way to move to golang/dep in the midterm future. Is this still a problem there?

Contributor

sttts commented Jul 21, 2017

if we plan to develop it in its own repo

In the worst case we can manually keep the main package in the vendor tree. It's a godep issue, not a vendor-experiment issue.

I hope we find a way to move to golang/dep in the midterm future. Is this still a problem there?

@mbohlool

This comment has been minimized.

Show comment
Hide comment
@mbohlool

mbohlool Jul 21, 2017

Member

Probably yes. Though I didn't give it a try. Problem is for any dependency system, the best way to build dependency tree is import clauses and you cannot import a main package. That is a go thing as far as I understand. Unless there is a way in golang/dep to specify manual dependencies.

Member

mbohlool commented Jul 21, 2017

Probably yes. Though I didn't give it a try. Problem is for any dependency system, the best way to build dependency tree is import clauses and you cannot import a main package. That is a go thing as far as I understand. Unless there is a way in golang/dep to specify manual dependencies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment