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

Enable update staging client in master. #39504

Closed
wants to merge 1 commit into from

Conversation

k82cn
Copy link
Member

@k82cn k82cn commented Jan 6, 2017

Enable update staging client in master.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 6, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Jan 6, 2017
@eparis eparis added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jan 24, 2017
@eparis
Copy link
Contributor

eparis commented Jan 24, 2017

/approve

@eparis
Copy link
Contributor

eparis commented Jan 24, 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 Jan 24, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2017
@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@deads2k deads2k added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Jan 24, 2017
@deads2k
Copy link
Contributor

deads2k commented Jan 24, 2017

Removing this from the merge queue until #39099 is settled.

@deads2k
Copy link
Contributor

deads2k commented Jan 24, 2017

@caesarxuchao This enables auto-updating. I think this should wait until we sort out how to handle client-go vendoring. I think it should be done as part of a sync to client-go.

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 25, 2017
@k8s-github-robot k8s-github-robot assigned spxtr and unassigned eparis and deads2k Jan 30, 2017
@deads2k
Copy link
Contributor

deads2k commented Jan 31, 2017

I think we need to split the copy script into a script that copies the API types from k8s.io/kubernetes and a script that generates the client directly into client-go. After that, we do NOT run the vendoring step in kubernetes. Instead, there should be a client-go sync script like we have for apimachinery. If a bot were to sync all repos against the same kubernetes, the vendor dir could be built during that step and it would always have a matching set and we have no cycles.

@caesarxuchao
Copy link
Member

The process you proposed will work if there are only api/ and apis/ under client-go/pkg/. Otherwise staging/client-go will not compile because these other packages under staging/client-go/pkg/ can be out of date.

I will write an alternative approach in kubernetes/client-go#78, let's see if it's more intuitive.

@k82cn k82cn force-pushed the enable_update_staging_client branch from d4f498c to 7789cf3 Compare February 1, 2017 10:00
@k8s-github-robot
Copy link

/lgtm cancel //PR changed after LGTM, removing LGTM. @deads2k @eparis @k82cn

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

The following people have approved this PR: k82cn

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

We suggest the following people:
cc @ixdy
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 removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2017
@k82cn
Copy link
Member Author

k82cn commented Feb 1, 2017

@caesarxuchao / @deads2k , thanks for your comments :). how about juts delete those lines comments?

@k8s-ci-robot
Copy link
Contributor

@k82cn: The following test(s) failed:

Test name Commit Details Rerun command
Jenkins GCE etcd3 e2e 7789cf3 link @k8s-bot gce etcd3 e2e test this

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.

@deads2k
Copy link
Contributor

deads2k commented Feb 1, 2017

The process you proposed will work if there are only api/ and apis/ under client-go/pkg/. Otherwise staging/client-go will not compile because these other packages under staging/client-go/pkg/ can be out of date.

Using go list you'd be able to pick up the dependencies (and transitives) too.

@caesarxuchao
Copy link
Member

godep save under the hood is the same as go list.

I'm not too excited about generating clients in the publishing robot, it makes client-go's publishing process different from other repos. I think this idea is easier to think about. (oh I see you commented that idea is good, I'll experiment it and push something tomorrow)

@caesarxuchao
Copy link
Member

I think mainly I need to update the publishing robot to checkout the latest apimachinary and vendor it before publishing to k8s.io/client-go. I'll code it up tmr. I'll clean up copy.sh as well, but maybe we are not on the same page about copy.sh, we'll see :)

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2017
@k82cn k82cn closed this Feb 13, 2017
@k82cn k82cn deleted the enable_update_staging_client branch May 16, 2022 06:30
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. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants