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

Stop putting env vars into BoundPods. #4359

Merged
merged 1 commit into from
Feb 12, 2015

Conversation

erictune
Copy link
Member

They will still show up in etcd. They never were available
through the API.

A subsequent PR(s) will rip out all BoundPods code.
Working in small increments.

This PR will cause users on lagging cloud providers
to not get env vars in their pods if they update to this code.
They have already been warned via email.

Removed unit tests of BasicBoundPodFactory.
There is adequate coverage in pkg/kubelet/kubelet_test.go.

@thockin
Copy link
Member

thockin commented Feb 11, 2015

LGTM

So nice.

@thockin
Copy link
Member

thockin commented Feb 11, 2015

Do the Red Hat folks have concerns?

@smarterclayton

@smarterclayton
Copy link
Contributor

None.

@erictune
Copy link
Member Author

not sure why integration test failed on go1.4 on travis. Works for me and shippable and travis+1.3

@saad-ali saad-ali added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2015
@saad-ali
Copy link
Member

@erictune Could you rebase please

They will still show up in etcd.  They never were available
through the API.

A subsequent PR(s) will rip out all BoundPods code.
Working in small increments.

This PR will cause users on lagging cloud providers
to not get env vars in their pods if they update to this code.
They have already been warned via email.

Removed unit tests of BasicBoundPodFactory.
There is adequate coverage in pkg/kubelet/kubelet_test.go.
@erictune
Copy link
Member Author

@saad-ali rebased

saad-ali added a commit that referenced this pull request Feb 12, 2015
Stop putting env vars into BoundPods.
@saad-ali saad-ali merged commit 3bc8f4e into kubernetes:master Feb 12, 2015
@zmerlynn
Copy link
Member

Does this close #4176?

@erictune
Copy link
Member Author

no

On Thu, Feb 12, 2015 at 11:19 AM, Zach Loafman notifications@github.com
wrote:

Does this close #4176
#4176?


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

@erictune
Copy link
Member Author

and by "no" I mean yes.

On Thu, Feb 12, 2015 at 11:21 AM, Eric Tune etune@google.com wrote:

no

On Thu, Feb 12, 2015 at 11:19 AM, Zach Loafman notifications@github.com
wrote:

Does this close #4176
#4176?


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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants