Render container stats and configs as yaml #444
Conversation
Signed-off-by: Tony-Sol <[email protected]>
|
Beast. |
|
@jesseduffield would you please take a look at this? |
|
Sorry for the late review, I actually much prefer the yaml appearance to the JSON appearance. I'm happy to make it yaml without it being configurable. Could you do that @tony-sol ? |
|
@jesseduffield mm, maybe just make yaml by default, but keep this setting? |
|
For rendering in a UI though, I think yaml makes way more sense. k9s for example uses yaml to display pod specs and to my knowledge doesn't offer a way of instead showing it in JSON. I reckon we just switch to yaml and make it configurable if people complain. In general I want to keep configuration to a minimum where possible as it's a maintenance burden |
|
Also, there is a way to make yaml output colorized, but it requires to add goccy/go-yaml dependency with other updates in go.mod Should we make this or common white text is just fine? 
|
|
Nice, let's add that dependency, it's great for readability |
|
Should i make colors fully customizable, set some pre-configured themes or just hardcode colors? |
|
hardcode :) |
|
I see a point of future improvements - make themes or something, to make ui look more smooth and less like christmas tree 😄 |
|
So, this is how it looks like now |
pkg/utils/utils.go
Outdated
| case "json": | ||
| return dataJSON, err | ||
| case "yaml": | ||
| // Use Unmarshal->Marshal hack to convert vendor-locked objects to YAML with same structure as JSON |
There was a problem hiding this comment.
Could you explain a bit more about what's happening here? What do you mean by vendor locked?
There was a problem hiding this comment.
Sure, container.Details declared in vendor/github.com/docker/docker/api/types/stats.go already has json tags, but not yaml.
So, in order to keep ./vendor/ files consistent and not changed in any inappropriate way, like
-- CPUUsage CPUUsage `json:"cpu_usage"`
++ CPUUsage CPUUsage `json:"cpu_usage" yaml:"cpu_usage"`and keep the resulting yaml structure exactly as json is, let's at first marshal into json as tagged, then into "any" yaml
There was a problem hiding this comment.
Ah, that makes sense. Could you capture that in a comment in the code?
| // Booleans: magenta | ||
| // Numbers: yellow | ||
| // Strings: green | ||
| func ColoredYamlString(str string) string { |
There was a problem hiding this comment.
Can you add a unit test for this? (Accidentally just left this as a general comment cos I'm reviewing from phone :) )
There was a problem hiding this comment.
@jesseduffield sure, done
There was a problem hiding this comment.
And sadly, i don't get about scrapping "the JSON-related code" :c
There was a problem hiding this comment.
What I mean is, let's remove DetailsFormat and always use yaml formatting.
|
@jesseduffield is everything good?, are we ready for merge?:) |
|
@tony-sol I've left a couple more comments :) |
`detailsFormat` key removed from config `MarshalIntoFormat` renamed into `marshalIntoFormat` with better description `MarshalIntoYaml` introduced as `marshalIntoFormat` wrapper for common usage
|
@jesseduffield done 😄 |
Yep that's fine. Merged! Great work on this @tony-sol ! |
jesseduffield
changed the title
json and yaml
Add optional config
".gui.detailsFormat"into config file, which provides ability to change output format of container stats and config details.To preserve current behaviour, use
jsonas default.Keys and their order are same between
jsonandyamlformats.