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

Add default imagePullPolicy for initContainers. #38574

Merged
merged 1 commit into from Jan 2, 2017
Merged

Add default imagePullPolicy for initContainers. #38574

merged 1 commit into from Jan 2, 2017

Conversation

k82cn
Copy link
Member

@k82cn k82cn commented Dec 11, 2016

fixes #38542

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 11, 2016
@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 Dec 11, 2016
@k82cn
Copy link
Member Author

k82cn commented Dec 11, 2016

I'm going to append test result later.

@k82cn k82cn changed the title [WIP] Add default imagePullPolicy for initContainers. Add default imagePullPolicy for initContainers. Dec 11, 2016
@k82cn
Copy link
Member Author

k82cn commented Dec 11, 2016

Test Cases: ReplicaSet's initContainers' without imagePullPolicy (refer to yml in #38542)
Test Steps:

Before:

$kubectl create -f 38542.yml
The ReplicaSet "nginx" is invalid: spec.template.spec.initContainers[0].imagePullPolicy: Required value

After:

$kubectl create -f 38542.yml
replicaset "nginx" created

@k8s-ci-robot
Copy link
Contributor

Jenkins CRI GCE e2e failed for commit 11e5da5. Full PR test history.

The magic incantation to run this job again is @k8s-bot cri e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@calebamiles
Copy link
Contributor

@k8s-bot cri e2e test this

@foxish
Copy link
Contributor

foxish commented Dec 14, 2016

This should be in the 1.5 milestone and cherrypicked I imagine.

cc @dims @saad-ali

@foxish foxish added this to the v1.5 milestone Dec 14, 2016
@jimmidyson jimmidyson mentioned this pull request Dec 15, 2016
Closed
@jstrachan jstrachan mentioned this pull request Dec 15, 2016
Closed
@k82cn
Copy link
Member Author

k82cn commented Dec 15, 2016

@k8s-bot cri e2e test this

@kingdonb
Copy link

Anyone know of a reason this hasn't been merged ?

@kingdonb
Copy link

Not sure if Jenkins CRI GCE e2e is broken upstream, I don't think the failure is as a result of this change though.

@k82cn
Copy link
Member Author

k82cn commented Dec 18, 2016

@k8s-bot cri e2e test this

1 similar comment
@k82cn
Copy link
Member Author

k82cn commented Dec 20, 2016

@k8s-bot cri e2e test this

@k8s-github-robot k8s-github-robot added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Dec 20, 2016
@k8s-github-robot
Copy link

This PR is not for the master branch but does not have the cherrypick-approved label. Adding the do-not-merge label.

smarterclayton
smarterclayton reviewed Dec 20, 2016
View reviewed changes
pkg/api/v1/conversion.go
@@ -493,6 +493,12 @@ func Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(in *PodTemplateSpec, out
// taking responsibility to ensure mutation of in is not exposed
// back to the caller.
in.Spec.InitContainers = values

// Call defaulters explicitly until annotations are removed
for i := range in.Spec.InitContainers {
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't get called by the kubelet - are the defaults being correctly applied there?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind - kubelet is non-defaulting now.

This won't work in all possible code paths in the api server, but is probably sufficient for now.

@smarterclayton
Copy link
Contributor

Please add a test case that demonstrates this.

@k82cn
Copy link
Member Author

k82cn commented Dec 21, 2016

Sure, I'll add a test case for that :).

Review status: 0 of 2 files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

pkg/api/v1/conversion.go, line 498 at r1 (raw file):

Previously, smarterclayton (Clayton Coleman) wrote…

Never mind - kubelet is non-defaulting now.

This won't work in all possible code paths in the api server, but is probably sufficient for now.

After defaulting in apiserver, the kubelet will get it in serialize/de-serialize.

Comments from Reviewable

@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 Dec 21, 2016
@k82cn
Copy link
Member Author

k82cn commented Dec 21, 2016

@k8s-bot verify test this

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 98bb7b5d48b74781270ad20315e9566e0c74190f. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@jstrachan jstrachan mentioned this pull request Dec 21, 2016
Closed
@k82cn
Copy link
Member Author

k82cn commented Dec 22, 2016

Added a test case for this issue.

/cc @smarterclayton

@k82cn
Copy link
Member Author

k82cn commented Dec 30, 2016

@smarterclayton , PTAL :).

@smarterclayton smarterclayton added release-note-none Denotes a PR that doesn't merit a release note. cherrypick-candidate and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. release-note-label-needed labels Dec 30, 2016
@smarterclayton smarterclayton modified the milestones: v1.5, v1.6 Dec 30, 2016
@smarterclayton
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 30, 2016
@smarterclayton smarterclayton added this to the v1.5 milestone Dec 30, 2016
@k8s-cherrypick-bot
Copy link

Removing label cherrypick-candidate because no release milestone was set. This is an invalid state and thus this PR is not being considered for cherry-pick to any release branch. Please add an appropriate release milestone and then re-add the label.

@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit cd6792a. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit cd6792a. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@smarterclayton
Copy link
Contributor

@k8s-bot gce etcd3 e2e test this

@k82cn
Copy link
Member Author

k82cn commented Jan 2, 2017

@k8s-bot bazel test this

@k8s-github-robot
Copy link

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit cc0b17e into kubernetes:master Jan 2, 2017
@k82cn k82cn deleted the k8s_38542 branch January 2, 2017 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@smarterclayton smarterclayton smarterclayton left review comments

Assignees

@smarterclayton smarterclayton

Labels
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-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.5
Development

Successfully merging this pull request may close these issues.

imagePullPolicy is required on Init Containers within a Pod template
10 participants
@k82cn @k8s-reviewable @k8s-ci-robot @calebamiles @foxish @kingdonb @k8s-github-robot @smarterclayton @k8s-cherrypick-bot @lavalamp