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

Show resource limits with describe #9304

Merged
merged 1 commit into from Jun 24, 2015
Merged

Conversation

jsdir
Copy link
Contributor

@jsdir jsdir commented Jun 5, 2015

Closes #9258

Signed-off-by: Jason Sommer <jsdirv@gmail.com>
@k8s-bot
Copy link

k8s-bot commented Jun 5, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@erictune
Copy link
Member

erictune commented Jun 5, 2015

ok to test

@k8s-bot
Copy link

k8s-bot commented Jun 5, 2015

EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded.

@ghost
Copy link

ghost commented Jun 5, 2015

@bgrant0607 @dchen1107 do either of you have bandwidth to review? I'm a bit snowed under right now.

@derekwaynecarr
Copy link
Member

I can review since I requested it

@derekwaynecarr derekwaynecarr self-assigned this Jun 5, 2015
@bgrant0607
Copy link
Member

Thanks, @derekwaynecarr

We're removing interpretContainerStatus from kubectl get pods in PR #9032. This looks at status.lastState, among other things. We should add equivalent info to describe. Please at least factor out the big switch on state, so that we could also use it to display lastState.

@derekwaynecarr
Copy link
Member

@jsdir - The change looked good when I tried it.

Name:               nginx-7wbly
Image(s):           nginx
Node:               10.245.1.3/10.245.1.3
Labels:             run=nginx
Status:             Running
Replication Controllers:    nginx (1/1 replicas created)
Containers:
  nginx:
    Image:  nginx
    Limits:
      cpu:      250m
      memory:       6Mi
    State:      Running
      Started:      Fri, 05 Jun 2015 16:12:38 -0400
    Ready:      True
    Restart Count:  0
Conditions:

@bgrant0607 - We can hold on this PR until #9032 lands.

@k8s-bot
Copy link

k8s-bot commented Jun 10, 2015

Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist")

If this message is too spammy, please complain to ixdy.

@derekwaynecarr
Copy link
Member

@bgrant0607 - this PR still has the big switch on state, but is an improvement on what existed previously for the end-user. are you fine taking as is and factoring out state in a follow-on?

@bgrant0607
Copy link
Member

Yes, I'm fine with refactoring in a separate PR

@derekwaynecarr derekwaynecarr added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2015
@bgrant0607
Copy link
Member

ok to test

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

Re-running Shippable.

Otherwise, LGTM.

cc @davidopp for another LGTM

@k8s-bot
Copy link

k8s-bot commented Jun 24, 2015

GCE e2e build/test passed for commit cea5aaa.

@davidopp
Copy link
Member

LGTM

@mbforbes
Copy link
Contributor

Shippable still hasn't started. Fingers crossed for soon...

mbforbes added a commit that referenced this pull request Jun 24, 2015
Show resource limits with describe
@mbforbes mbforbes merged commit e44f619 into kubernetes:master Jun 24, 2015
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.

None yet

8 participants