RedHatSatellite / soe-ci

Jenkins integration scripts for use in an SOE Build Factory based on Red Hat Satellite 6.
GNU General Public License v3.0
42 stars 32 forks source link

fold at 240 columns instead of 80 #92

Closed pcfe closed 7 years ago

pcfe commented 7 years ago

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 ;-)

nstrug commented 7 years ago

Can you explain to me what this does and why?

pcfe commented 7 years ago

@nstrug Eric's 9b5a5e16e5caba46ed9bbbf9d7e9e076a1450428 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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@ericzolf @nstrug

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