Skip to content
This repository has been archived by the owner on Sep 12, 2023. It is now read-only.

fold at 240 columns instead of 80 #92

Merged
merged 1 commit into from
Nov 2, 2017
Merged

fold at 240 columns instead of 80 #92

merged 1 commit into from
Nov 2, 2017

Conversation

pcfe
Copy link
Collaborator

@pcfe pcfe commented Oct 1, 2017

addresses #91
but I'd like a second opinion on my choice of 240, I'd be fine with anything from 120 upwards.
Before you ask; parametrising this only makes sense once we move to a proper pipeline ;-)

Copy link
Collaborator

@opuk opuk left a comment

Choose a reason for hiding this comment

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

I don't have any issues with this.

@nstrug
Copy link
Member

nstrug commented Oct 6, 2017

Can you explain to me what this does and why?

@pcfe
Copy link
Collaborator Author

pcfe commented Oct 6, 2017

@nstrug
Eric's 9b5a5e1 introduced tell() which uses fold(1).

He is using the break at spaces option (useful) but no column width, so it breaks after the default of maximum 80 character. That is pretty narrow on any display I use and I guess most other people having to dig into the logs of a jenkins job that failed.

The PR simply makes output, that went through the tell() function of common.sh, wrap at max 240 columns, thus making the logs more readable.

Example log extract folding at the default of 80

#####################################################
#            PUSHING TESTS to TEST VMS              #
#####################################################
INFO:    Waiting 10 seconds [Thu Oct  5 06:33:50 CEST 2017]
INFO:    Checking if test server sattestclient01.sattest.pcfe.net has rebooted 
into OS so that tests can be run [Thu Oct  5 06:34:00 CEST 2017]
INFO:    note that the Jenkins job definition may trigger a fresh installation, 
[Thu Oct  5 06:34:00 CEST 2017]
INFO:    so check the console of sattestclient01.sattest.pcfe.net if this step 
is taking too long. [Thu Oct  5 06:34:00 CEST 2017]
Not yet. [Thu Oct  5 06:34:04 CEST 2017]
INFO:    Waiting 10 seconds [Thu Oct  5 06:34:04 CEST 2017]
INFO:    Checking if test server sattestclient01.sattest.pcfe.net has rebooted 
into OS so that tests can be run [Thu Oct  5 06:34:14 CEST 2017]

When folding at 240, one would get the more readable

#####################################################
#            PUSHING TESTS to TEST VMS              #
#####################################################
INFO:    Waiting 10 seconds [Thu Oct  5 06:33:50 CEST 2017]
INFO:    Checking if test server sattestclient01.sattest.pcfe.net has rebooted into OS so that tests can be run [Thu Oct  5 06:34:00 CEST 2017]
INFO:    note that the Jenkins job definition may trigger a fresh installation, [Thu Oct  5 06:34:00 CEST 2017]
INFO:    so check the console of sattestclient01.sattest.pcfe.net if this step is taking too long. [Thu Oct  5 06:34:00 CEST 2017]
Not yet. [Thu Oct  5 06:34:04 CEST 2017]
INFO:    Waiting 10 seconds [Thu Oct  5 06:34:04 CEST 2017]
INFO:    Checking if test server sattestclient01.sattest.pcfe.net has rebooted into OS so that tests can be run [Thu Oct  5 06:34:14 CEST 2017]

@jeichler
Copy link
Collaborator

let me chime in here. I don't think parametrizing the line break at $width makes sense. at least it's nothing a pipeline should take care of. it basically has nothing to do with the build itself.
it's rather an option for ansible if it's really desired, but seems a little over-engineered. when in doubt that's a value the customer could change easily (or we could use bash parameter substitution and simply assign a default if not set).

@ericzolf
Copy link
Collaborator

Why not something like: echo "${@} [$(date)]" | fold --spaces ${COLUMNS:+--width=${COLUMNS}} ?
It would work correctly out of the box on the command line and COLUMNS can surely be defined somewhere, as @jeichler wrote.
The default is still 80 but there is a reason I chose it initially, I don't like long lines :-)

@pcfe
Copy link
Collaborator Author

pcfe commented Oct 15, 2017

@nstrug PR as is fine with you or would you rather have what @ericzolf suggested?

@ericzolf since the parts of the logs that do not go through tell() are not wrapped at 80 columns, if we wrap as early as 80 columns, even on my portrait screen it disrupts rapid reading (Querlesen in German) of logs, I've temporarily made available an image and highlighted the lines that disrupt my reading flow, does that make my case clearer?
If you still feel strongly about wrapping at 80, then I'll add what you suggested next time I have spare cycles.

@pcfe
Copy link
Collaborator Author

pcfe commented Oct 30, 2017

@ericzolf
@nstrug

If I get no updates from you two by Wednesday then I'll merge this as is.

@pcfe pcfe merged commit 3ee563e into master Nov 2, 2017
@pcfe pcfe deleted the fold240 branch November 2, 2017 15:08
pcfe added a commit that referenced this pull request Nov 3, 2017
it's now Thursday, merging as is.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants