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

Removing ContainerManifest #9807

Merged
merged 1 commit into from Jun 25, 2015
Merged

Removing ContainerManifest #9807

merged 1 commit into from Jun 25, 2015
+26 −360

Conversation

krousey
Copy link
Contributor

@krousey krousey commented Jun 15, 2015

#8087

cc @caesarxuchao @nikhiljindal @bgrant0607

@dchen1107 Please double check pkg/kubelet/config/file_test.go and other kubelet changes.

@k8s-bot
Copy link

k8s-bot commented Jun 15, 2015

GCE e2e build/test passed for commit 18b78f8a792da6855a99c7a22a3b2a55ae50fef8.

caesarxuchao
caesarxuchao reviewed Jun 15, 2015
View reviewed changes
pkg/api/validation/validation.go
@@ -863,27 +863,6 @@ func validateContainers(containers []api.Container, volumes util.StringSet) errs

var supportedManifestVersions = util.NewStringSet("v1beta1", "v1beta2")
Copy link
Member

Choose a reason for hiding this comment

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

This line can be removed as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

caesarxuchao
caesarxuchao reviewed Jun 16, 2015
View reviewed changes
pkg/api/serialization_test.go
@@ -125,7 +125,7 @@ func TestList(t *testing.T) {
roundTripSame(t, item)
}

var nonRoundTrippableTypes = util.NewStringSet("ContainerManifest", "ContainerManifestList")
var nonRoundTrippableTypes = util.NewStringSet()
var nonInternalRoundTrippableTypes = util.NewStringSet("List", "ListOptions", "PodExecOptions")
Copy link
Member

Choose a reason for hiding this comment

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

@nikhiljindal @bgrant0607 , can we remove nonRoundTrippableTypes? Or are we expecting there will be new member in this sets in the future?

Copy link
Member

Choose a reason for hiding this comment

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

cc @smarterclayton

@dchen1107
Copy link
Member

I only reviewed kubelet related pieces, that lgtm.

Thanks for cleaning up.

@k8s-bot
Copy link

k8s-bot commented Jun 17, 2015

GCE e2e build/test passed for commit ea5861073f883f9d289e4d3922fa158575d40734.

@krousey
Copy link
Contributor Author

krousey commented Jun 18, 2015

I'm OOO right now, but occasionally checking in on this one. Is there anything left that I need to do?

@caesarxuchao
Copy link
Member

I'm still not sure about nonRoundTrippableTypes, but it shouldn't prevent this PR to be merged. Thank you. LGTM.

@caesarxuchao caesarxuchao added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed cla: yes labels Jun 18, 2015
@bgrant0607
Copy link
Member

I think it's fine to leave nonRoundTrippableTypes in the code for now.

@satnam6502
Copy link
Contributor

Milestone?

@caesarxuchao
Copy link
Member

@bgrant0607 could you set triage it? I don't think I have the right to set the milestone.

@bgrant0607 bgrant0607 added this to the v1.0 milestone Jun 19, 2015
@bgrant0607
Copy link
Member

This is ok to go in, but the test failures need to be fixed.

@bgrant0607 bgrant0607 removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 19, 2015
@krousey
Copy link
Contributor Author

krousey commented Jun 19, 2015

The only test failure I saw was what looked to be a flaky shippable build,
and I couldn't figure out how to rerun it. Did the e2e fail? I'm on my
phone and can't really see the test statuses.
On Jun 18, 2015 5:58 PM, "Brian Grant" notifications@github.com wrote:

This is ok to go in, but the test failures need to be fixed.

—
Reply to this email directly or view it on GitHub
#9807 (comment)
.

@satnam6502
Copy link
Contributor

@k8s-bot ok to test

@k8s-bot
Copy link

k8s-bot commented Jun 19, 2015

GCE e2e build/test passed for commit ea5861073f883f9d289e4d3922fa158575d40734.

@caesarxuchao
Copy link
Member

Close and re-open the PR to trigger shippable

@caesarxuchao caesarxuchao reopened this Jun 19, 2015
@krousey
Copy link
Contributor Author

krousey commented Jun 19, 2015

Rebased

@k8s-bot
Copy link

k8s-bot commented Jun 20, 2015

GCE e2e build/test passed for commit 3573e744eee8a4b1d46b5155c2b45c65d77b11ca.

krousey
krousey reviewed Jun 24, 2015
View reviewed changes
contrib/mesos/pkg/executor/executor_test.go
"kind": "Pod",
"metadata": {
"name": "%v",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jdef FYI I had to fix this error in Pod specification. By removing the name defaulting in pkg/kubelet/config/common.go, this test started failing.

@krousey
Copy link
Contributor Author

krousey commented Jun 24, 2015

I fixed the mesos test failure that relied on Pod name defaulting logic in kubelet.

@k8s-bot
Copy link

k8s-bot commented Jun 24, 2015

GCE e2e build/test passed for commit d13421e.

@caesarxuchao
Copy link
Member

Thanks @krousey. LGTM. @bgrant0607, could you give the second LGTM? Thanks.

bgrant0607
bgrant0607 reviewed Jun 25, 2015
View reviewed changes
contrib/mesos/pkg/executor/executor_test.go
@@ -437,9 +437,9 @@ func TestExecutorStaticPods(t *testing.T) {
assert.NoError(t, err)
spod := `{
"apiVersion": "v1beta3",
Copy link
Member

Choose a reason for hiding this comment

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

Could we just change this to v1 while we're touching it?

@bgrant0607
Copy link
Member

LGTM. We can do the v1beta3->v1 conversion later, if necessary.

@bgrant0607 bgrant0607 added lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-merge labels Jun 25, 2015
mbforbes added a commit that referenced this pull request Jun 25, 2015
@mbforbes
Merge pull request #9807 from krousey/container_manifest
2894676 
Removing ContainerManifest
@mbforbes mbforbes merged commit 2894676 into kubernetes:master Jun 25, 2015
@krousey krousey deleted the container_manifest branch August 21, 2015 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@caesarxuchao caesarxuchao

Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Milestone
v1.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@krousey @k8s-bot @dchen1107 @caesarxuchao @bgrant0607 @satnam6502 @mbforbes @googlebot