search

RedHatInsights / yggdrasil

GNU General Public License v3.0
21 stars 37 forks source link

feat(journal): add "truncate-field"/"truncate-all-fields" flags #164

Open DuckBoss opened 1 year ago

DuckBoss commented 1 year ago

Do not merge before: https://github.com/RedHatInsights/yggdrasil/pull/116

This feature implements the --truncate-field/--tf and --truncate--all-fields/--taf flags. The --truncate-field flag allows users to truncate worker data content for the specified field to the character count provided. The --truncate-all-fields flag allows users to truncate all worker data content fields to the character count provided.

The --truncate-field flag can be used multiple times to truncate content for multiple fields.

Example usage:

Regular usage w/o truncating any data fields: Input: yggctl message-journal Output: MESSAGE # MESSAGE ID ... WORKER DATA
0 message-id-0 ... ...
1 message-id-1 ... {"message": "echoing data 1 2 3"}
2 message-id-2 ... ...
Usage w/ truncating a single data field: Input: yggctl message-journal --truncate-field message=5 Output: MESSAGE # MESSAGE ID ... WORKER DATA
0 message-id-0 ... ...
1 message-id-1 ... {"message": "echoi..."}
2 message-id-2 ... ...
Usage w/ truncating multiple data fields: Input: yggctl message-journal --tf fieldA=2 --tf fieldB=1 --tf fieldC=5 Output: MESSAGE # MESSAGE ID ... WORKER DATA
0 message-id-0 ... ...
1 message-id-1 ... {"fieldA": "ec...", "fieldB": "e...", "fieldC": "echoi..."}
2 message-id-2 ... ...
Usage w/ truncating ALL data fields: Input: yggctl message-journal --taf 5 Output: MESSAGE # MESSAGE ID ... WORKER DATA
0 message-id-0 ... ...
1 message-id-1 ... {"fieldA": "echoi...", "fieldB": "echoi...", "fieldC": "echoi..."}
2 message-id-2 ... ...
subpop commented 9 months ago

Is the WORKER DATA field always JSON?

Edit: confirmed for myself; yes, the WorkerEvent.Data field is always parsable as JSON (Go declares it as map[string]string).

subpop commented 9 months ago

So there's just something odd about these flags that I can't put into words yet. I looked over the CLIG and read the section on command line arguments and flags. It links to some of its source material, which includes an article on good CLI design decisions. This article has a section on table usage. This section talks about output formatting, and includes the following:

Be mindful of the screen width. Only show a few columns by default but allow the user to pass --columns with a comma-separated list of column names to add less common types. Truncate rows that are going to spill over the current screen width unless --no-truncate is set.

Because the table is an interactive output format, perhaps we should default to dynamic truncation, and allow the user to simply pass a --no-truncate flag if they want to skip the truncation. This does require a rather significant rework of your PR though, but since we never went through any sort of design discussion, I think it's worth having before we get too far into the implementation specifics.

DuckBoss commented 9 months ago

Thanks for the resources, that's definitely a fundamental change to this PR and I agree this is worth having a design discussion first.

subpop commented 7 months ago

I put some thought into this finally. And started a discussion about it (#215).