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 more detailed pod status on pod list page #1308

Merged
merged 8 commits into from
Oct 7, 2016

Conversation

ianlewis
Copy link
Contributor

@ianlewis ianlewis commented Oct 6, 2016

This PR is meant to address #1121. Changed status on the pod list page to show the same status as kubectl.

Future work needed:

  • Logic for whether to show the pod as successful or not needs to be updated to incorporate status.containerStatuses and/or status.conditions.
  • Messages on the pod list page need to be updated to reflect new success logic.
  • More info on container status needs to be shown on the Pod detail page.
  • Make use of future printer API work? (WIP: Proposal for moving "printers" (kubectl get) to the server kubernetes#29472)

@codecov-io
Copy link

codecov-io commented Oct 6, 2016

Current coverage is 93.71% (diff: 87.50%)

Merging #1308 into master will decrease coverage by 0.05%

@@             master      #1308   diff @@
==========================================
  Files           342        342          
  Lines          3080       3101    +21   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2888       2906    +18   
- Misses          192        195     +3   
  Partials          0          0          

Powered by Codecov. Last update 27480e1...cc4588b

}
}
if pod.DeletionTimestamp != nil {
reason = "Terminating"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have thoughts on internationalizing this? Entire UI is translated, but messages from the server aren't. Any solutions to this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yah. This is a difficult one. Kubectl displays internal error names/strings rather than "messages" in the translatable sense. I'm not sure it's really translatable unless we break with kubectl and show something like "Error: %(reason)s" or something like that.

Perhaps that's a better approach?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I meant by that is that we could translate "Terminating" and other statuses we make up here, but sometimes it would show the "Reason" field from failed container statuses returned via the API and those won't be translatable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes. I think that's best solution. With the new i18n framework you could do this in the HTML via:
[[Error: {{$ctrl.errorReason}}|A description]]

@@ -30,14 +32,77 @@ func getRestartCount(pod api.Pod) int32 {
return restartCount
}

// getDisplayPods gets the status of the pod for display in a compact view like a list view. Logic is copied from kubectl ResourcePrinter implementation.
func getDisplayStatus(pod api.Pod) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we have tests for this fuction? Looks like an easy candidate for a few unit tests.

@ianlewis ianlewis changed the title Added display status Show more detailed pod status on pod list page Oct 6, 2016
@ianlewis
Copy link
Contributor Author

ianlewis commented Oct 6, 2016

Updated to have better messages that are translatable. Tests will be forthcoming.

@bryk
Copy link
Contributor

bryk commented Oct 7, 2016

All right. Looks good. Please add tests now :)

@ianlewis
Copy link
Contributor Author

ianlewis commented Oct 7, 2016

The linters... They hate me...

@ianlewis
Copy link
Contributor Author

ianlewis commented Oct 7, 2016

@bryk Added tests and tamed the linters/compilers. PTAL.

Copy link
Contributor

@bryk bryk left a comment

Choose a reason for hiding this comment

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

Great, LGTM :)

@bryk bryk merged commit 09ce772 into kubernetes:master Oct 7, 2016
@ianlewis ianlewis deleted the pod-display-status branch February 23, 2017 05:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants