-
Notifications
You must be signed in to change notification settings - Fork 369
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
feat: add output format flag to the k0s sysinfo command #5264
base: main
Are you sure you want to change the base?
Conversation
Ha, wanted to do this since its inception ^^ The initial idea was to have a separate JSON reporter for this, though. |
@twz123 Could you provide some feedback on the Probe struct? After that I'm happy to refactor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the JOSN/YAML structure. Did you consider to model the sysinfo hierarchy into the output structure? Alternatively, have the paths be the keys in a big map (urgh, there's no way to keep the order of things in there, right? :-( )? I'd also use camelCase for the keys, and omit empty fields. I'm pondering about a "somewhat natural" jq/jsonpointer expression to select individual probes 🤔
cmd/sysinfo/sysinfo.go
Outdated
w io.Writer | ||
colors aurora.Aurora | ||
failed bool | ||
type cliReporter interface { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I had in mind:
- Leave the
cliReporter
unmodified. - Just have the
resultsCollector
to collect the tree.
Have the switch statement in the command look like this:
switch outputFormat {
case "human":
cli := &cliReporter{
w: out,
colors: aurora.NewAurora(term.IsTerminal(out)),
}
if err := probes.Probe(cli); err != nil {
return err
}
if cli.failed {
return errors.New("sysinfo failed")
}
return nil
case "json":
return collectAndPrint(probes, out, json.Marshal)
case "yaml":
return collectAndPrint(probes, out, yaml.Marshal)
default:
return fmt.Errorf("unknown output format: %q", outputFormat)
}
And the collectAndPrint
function could look like this:
func collectAndPrint(probe probes.Probe, out io.Writer, marshal func(any) ([]byte, error)) error {
var c resultsCollector
if err := probe.Probe(&c); err != nil {
return err
}
if c.failed {
return errors.New("sysinfo failed")
}
bytes, err := marshal(c.results)
if err != nil {
return err
}
_, err = out.Write(bytes)
return err
}
cmd/sysinfo/sysinfo.go
Outdated
@@ -64,68 +80,177 @@ func NewSysinfoCmd() *cobra.Command { | |||
flags.BoolVar(&sysinfoSpec.ControllerRoleEnabled, "controller", true, "Include controller-specific sysinfo") | |||
flags.BoolVar(&sysinfoSpec.WorkerRoleEnabled, "worker", true, "Include worker-specific sysinfo") | |||
flags.StringVar(&sysinfoSpec.DataDir, "data-dir", constant.DataDirDefault, "Data Directory for k0s") | |||
flags.StringVarP(&outputFormat, "output", "o", "human", "Output format. Must be one of human|yaml|json") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how it's done in similar flags on other subcommands.
flags.StringVarP(&outputFormat, "output", "o", "human", "Output format. Must be one of human|yaml|json") | |
flags.StringVarP(&outputFormat, "output", "o", "human", "Output format (valid values: human, json, yaml)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated although I copied the flag from here.
cmd/sysinfo/sysinfo.go
Outdated
|
||
var cli cliReporter | ||
switch outputFormat { | ||
case "human": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "text"
or "cli"
?
640ffdd
to
c5afe54
Compare
Signed-off-by: Ethan Mosbaugh <[email protected]>
Signed-off-by: Ethan Mosbaugh <[email protected]>
Signed-off-by: Ethan Mosbaugh <[email protected]>
Signed-off-by: Ethan Mosbaugh <[email protected]>
Signed-off-by: Ethan Mosbaugh <[email protected]>
f0b3ca5
to
c8f7089
Compare
For nested, how about something like this? {
"results": {
"os": {
"displayName": "Operating system",
"prop": "Linux",
"message": "",
"category": "pass",
"error": null,
"results": {
"net": {
"displayName": "CONFIG_NET: Networking support",
"prop": "built-in",
"message": "",
"category": "pass",
"error": null,
"results": {
"bridge": {
"displayName": "CONFIG_BRIDGE: 802.1d Ethernet Bridging",
"prop": "built-in",
"message": "",
"category": "pass",
"error": null,
"results": {
"stp": {
"displayName": "CONFIG_LLC",
"prop": "built-in",
"message": "",
"category": "pass",
"error": null
}
}
}
}
}
}
}
}
} Corresponding jq
Alternatively, for a simpler approach and one that is ordered, how about joining the path with dot? [
{
"path": "os.NET.BRIDGE.STP",
"displayName": "CONFIG_LLC",
"prop": "built-in",
"message": "",
"category": "pass",
"error": null
}
] jq
|
Signed-off-by: Ethan Mosbaugh <[email protected]>
Description
Adds --output flag to the sysinfo command with formats human|yaml|json.
sysinfo.json
Fixes # (issue)
Type of change
How Has This Been Tested?
Checklist: