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

list: add ip of non-running pods to status output #3857

Merged
merged 4 commits into from Jan 10, 2018

Conversation

Projects
None yet
3 participants
@sch00lb0y
Contributor

sch00lb0y commented Nov 19, 2017

partial fix for #3550
ip of the non running pod is added to the json output

@squeed

This comment has been minimized.

Contributor

squeed commented Nov 20, 2017

What happens if you change https://github.com/rkt/rkt/blob/master/pkg/pod/pods.go?utf8=%E2%9C%93#L272 from if p.isRunning() to if p.isRunning() || p.isExited()? Would that accomplish the same thing?

@sch00lb0y

This comment has been minimized.

Contributor

sch00lb0y commented Nov 20, 2017

@squeed thanks for reviewing the PR. I have made changes as you suggested.

@squeed

This comment has been minimized.

Contributor

squeed commented Nov 21, 2017

Works for me. Can you add a test to rkt_list_test.go? Then this can be merged.

{
"list --format=json",
true,
podUuid

This comment has been minimized.

@iaguis

iaguis Dec 18, 2017

Member

Can you add a comma here so tests pass?

@sch00lb0y

This comment has been minimized.

Contributor

sch00lb0y commented Dec 27, 2017

@squeed I have added the test case. @iaguis I have made the changes. the test is passing in my local environment but it's failing in the semaphore

@iaguis

This comment has been minimized.

Member

iaguis commented Jan 2, 2018

It seems your change breaks TestAPIServiceCgroup. The reason is that we now consider network information for exited pods but the API service doesn't, so the test fails because the info returned by the API service doesn't match the one returned by rkt status.

We should modify the API service to populate network information on exited pods, like we do for running ones.

@sch00lb0y

This comment has been minimized.

Contributor

sch00lb0y commented Jan 3, 2018

@iaguis Thanks for helping me out. Now the test is passing. please review the PR.

@iaguis

iaguis approved these changes Jan 3, 2018

lgtm

But can you make your commit follow our convention?

We also try to prefer imperative in titles: like "add ip of non running"...

Thanks!

@squeed

squeed approved these changes Jan 3, 2018

@lucab lucab changed the title from list: ip of non running pod is added to the json output to list: add ip of non-running pods to status output Jan 8, 2018

@sch00lb0y

This comment has been minimized.

Contributor

sch00lb0y commented Jan 10, 2018

@iaguis I have made changes, please check

@iaguis

iaguis approved these changes Jan 10, 2018

Thanks!

@iaguis iaguis merged commit 6d32c3f into rkt:master Jan 10, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details

@iaguis iaguis added this to the 1.30.0 milestone Apr 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment