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

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DuckBoss
Copy link
Contributor

@DuckBoss DuckBoss commented Sep 22, 2023

Do not merge before: #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 ... ...

@DuckBoss DuckBoss changed the title feat(journal): add "truncate-field" flag [WIP] feat(journal): add "truncate-field" flag Sep 22, 2023
@DuckBoss DuckBoss marked this pull request as draft September 22, 2023 19:49
@DuckBoss DuckBoss force-pushed the jajerome/journal-truncate-fields branch from f465402 to 25a5828 Compare September 29, 2023 19:13
@DuckBoss DuckBoss changed the title [WIP] feat(journal): add "truncate-field" flag [WIP] feat(journal): add "truncate-field"/"truncate-all-fields" flags Sep 29, 2023
@DuckBoss DuckBoss force-pushed the jajerome/journal-truncate-fields branch 3 times, most recently from 816ed09 to 2e23130 Compare October 6, 2023 15:20
Signed-off-by: Jason Jerome <jajerome@redhat.com>
Signed-off-by: Jason Jerome <jajerome@redhat.com>
@DuckBoss DuckBoss force-pushed the jajerome/journal-truncate-fields branch from 2e23130 to e1c1beb Compare January 9, 2024 20:21
@DuckBoss DuckBoss changed the title [WIP] feat(journal): add "truncate-field"/"truncate-all-fields" flags feat(journal): add "truncate-field"/"truncate-all-fields" flags Jan 9, 2024
@DuckBoss DuckBoss marked this pull request as ready for review January 9, 2024 22:28
@DuckBoss DuckBoss requested a review from subpop January 9, 2024 22:29
@subpop
Copy link
Collaborator

subpop commented Jan 10, 2024

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 subpop self-assigned this Jan 10, 2024
@subpop
Copy link
Collaborator

subpop commented Jan 10, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

subpop commented Mar 14, 2024

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

@DuckBoss DuckBoss marked this pull request as draft September 4, 2024 15:37
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.

2 participants