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.

Sign up for GitHub

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

Jump to bottom

Use unversioned client in scheduledjobs and set group version to batch/v2alpha1 #30327

Merged
merged 1 commit into from
Aug 13, 2016
Merged

Use unversioned client in scheduledjobs and set group version to batch/v2alpha1 #30327

merged 1 commit into from
Aug 13, 2016
+18 −12

Conversation

janetkuo
Copy link
Member

@janetkuo janetkuo commented Aug 10, 2016 •
edited by k8s-oncall
Loading

Fixes #30323

This change is Reviewable

@janetkuo janetkuo added kind/bug Categorizes issue or PR as related to a bug. area/batch labels Aug 10, 2016
@janetkuo janetkuo added this to the v1.4 milestone Aug 10, 2016
@janetkuo janetkuo added the release-note-none Denotes a PR that doesn't merit a release note. label Aug 10, 2016
@k8s-github-robot k8s-github-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 10, 2016
@janetkuo janetkuo mentioned this pull request Aug 10, 2016
Merged
@soltysh
Copy link
Contributor

soltysh commented Aug 10, 2016

I'll start with fixing this with #25442. The change itself LGTM.

@soltysh soltysh added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 10, 2016
soltysh
soltysh reviewed Aug 10, 2016
View reviewed changes
cmd/kube-controller-manager/app/controllermanager.go
@@ -128,6 +129,7 @@ func Run(s *options.CMServer) error {
}

kubeconfig.ContentConfig.ContentType = s.ContentType
kubeconfig.ContentConfig.GroupVersion = &unversioned.GroupVersion{Group: batch.GroupName, Version: "v2alpha1"}
Copy link
Contributor

Choose a reason for hiding this comment

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

On second thought, can you move this piece and initiate the kubeclient with this version only when batch/v2alpha1 is enabled?

Copy link
Contributor

Choose a reason for hiding this comment

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

Iow. just before starting the controller, you do have access to kubeconfig at that point and should be able to create a new kubeclient easily.

@soltysh
Copy link
Contributor

soltysh commented Aug 10, 2016

As stated before LGTM, except for the single nit which when you fix you can apply the label.

@janetkuo janetkuo force-pushed the sj-controller-client branch from d718b45 to 04e8211 Compare August 10, 2016 17:45
@janetkuo janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 10, 2016
@k8s-github-robot k8s-github-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 Aug 10, 2016
@janetkuo janetkuo force-pushed the sj-controller-client branch from 04e8211 to 5984f5e Compare August 10, 2016 17:49
@janetkuo janetkuo added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 10, 2016
@janetkuo janetkuo closed this Aug 10, 2016
@janetkuo janetkuo reopened this Aug 10, 2016
@janetkuo
Copy link
Member Author

@k8s-bot test this issue: #27462

@janetkuo
Copy link
Member Author

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

cmd/kube-controller-manager/app/controllermanager.go, line 132 [r1] (raw file):

Previously, soltysh (Maciej Szulik) wrote…

Iow. just before starting the controller, you do have access to kubeconfig at that point and should be able to create a new kubeclient easily.

Done.

Comments from Reviewable

@janetkuo
Copy link
Member Author

@soltysh looks like you need to accept the change in reviewable otherwise the bot won't merge this (see the code-review/reviewable — 3 files, 1 discussion left (soltysh) check)

@soltysh
Copy link
Contributor

soltysh commented Aug 10, 2016

LGTM

Comments from Reviewable

@soltysh
Copy link
Contributor

soltysh commented Aug 10, 2016

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from Reviewable

@soltysh
Copy link
Contributor

soltysh commented Aug 11, 2016

@k8s-bot test this issue: #30377

@soltysh soltysh mentioned this pull request Aug 11, 2016
Merged
@soltysh
Copy link
Contributor

soltysh commented Aug 12, 2016

@k8s-bot test this issue: #30377

@soltysh
Copy link
Contributor

soltysh commented Aug 12, 2016

@janetkuo I had one of the previous versions of this patch in #26027 which merged just now, can you update this PR with the necessary changes, currently it'll definitely conflict.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2016
@janetkuo janetkuo force-pushed the sj-controller-client branch from 5984f5e to b948375 Compare August 12, 2016 22:39
@janetkuo janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Aug 12, 2016
@janetkuo
Copy link
Member Author

Rebased

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 12, 2016
@girishkalele
Copy link

Hopefully this commit goes in soon...the Submit Queue is blocked due to the GKE failures from #26027

@ixdy
Copy link
Member

ixdy commented Aug 12, 2016

@girishkalele you're the buildcop, right? you can just merge it if you are satisfied it's good.

@ixdy
Copy link
Member

ixdy commented Aug 12, 2016

(but maybe at least wait for passing PR Jenkins)

@girishkalele
Copy link

@janetkuo @ixdy We are not sure if this will fix the other e2e failures for sure.

I will revert the e2es and clear the GKE blocked tests first and then we can resubmit the reverted PR.

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2016
@janetkuo janetkuo force-pushed the sj-controller-client branch from b948375 to e4269d4 Compare August 12, 2016 23:58
@janetkuo janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 12, 2016
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 13, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 13, 2016

GCE e2e build/test passed for commit e4269d4.

@k8s-github-robot
Copy link

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

@k8s-bot
Copy link

k8s-bot commented Aug 13, 2016

GCE e2e build/test passed for commit e4269d4.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit dadb332 into kubernetes:master Aug 13, 2016
k8s-github-robot pushed a commit that referenced this pull request Aug 14, 2016
Merge pull request #30451 from soltysh/isssue30442
85c91eb 
Automatic merge from submit-queue

Remove pods along with jobs when Replace ConcurrentPolicy is set

Fixes #30442

This builds on #30327 and needs a bit more love in tests.

@janetkuo @erictune fyi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@soltysh soltysh

Labels
area/batch kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
7 participants
@janetkuo @soltysh @girishkalele @ixdy @k8s-bot @k8s-github-robot @googlebot