Some functions of liblog.sh log component are enhanced #76245

huangjc7 · 2025-01-14T16:18:11Z

Added BITNAMI_COLOR method, which can control the color switch
Signed-off-by: huangjc7 [email protected]

Description of the change

Added errorX function, which exits with non-zero value after execution
Added BITNAMI_COLOR method, which can control the color switch

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

huangjc7 · 2025-01-15T02:24:59Z

@gongomgra VIB verification failed. What is the reason?

gongomgra

Thanks for your contribution! I have added some comments to your changes

gongomgra · 2025-01-21T15:45:30Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

+        reset_color=""
+    fi
+    log "${msg_color}ERROR${reset_color} ==> ${*}"
+    exit 1


Notice this is a logging library, and that running exit 1 here doesn't make much sense. Please remove this method.

Do not disregard this comment, please.

gongomgra · 2025-01-21T15:53:50Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

 log() {
-    stderr_print "${CYAN}${MODULE:-} ${MAGENTA}$(date "+%T.%2N ")${RESET}${*}"
+    local prefix=""
+    local suffix=""
+    local color_check="${BITNAMI_COLOR:-true}"
+    if [[ "$color_check" =~ ^(yes|true)$ ]]; then
+        prefix="${CYAN}${MODULE:-} ${MAGENTA}$(date "+%T.%2N ")${RESET}"
+        suffix="${RESET}"
+    else
+        prefix="${MODULE:-} $(date "+%T.%2N ")"
+    fi
+    stderr_print "${prefix}${*}${suffix}"
 }


I think a couple of minor changes in this method would make it clear, what do you think?

Rename color_check to color_bool.

Update if logic to match the one in line 29 of this file due to the reasons in line 25.

gongomgra · 2025-01-21T15:54:08Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

+    local color_check="${BITNAMI_COLOR:-true}"
+    if ! [[ "$color_check" =~ ^(yes|true)$ ]]; then


Update if condition format, and please implement the logic the other way around: if true then set colors

gongomgra · 2025-01-21T15:56:54Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

+        msg_color=""
+        reset_color=""
+    fi
+    log "${msg_color}INFO ${reset_color} ==> ${*}"


I would simply use ${RESET} here (for all use cases).

gongomgra · 2025-01-21T15:58:46Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

+    local msg_color="$YELLOW"
+    local reset_color="$RESET"
+    local color_check="${BITNAMI_COLOR:-true}"
+    if ! [[ "$color_check" =~ ^(yes|true)$ ]]; then


Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

gongomgra

Thanks for the update. Please check the pending comments and also update the conditionals on booleans variables to match the if structure in line 88 (check for 1 OR yes|true):

if [[ "$bool" = 1 || "$bool" =~ ^(yes|true)$ ]]: then
    ...
fi

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

huangjc7 · 2025-01-29T05:51:52Z

Thanks for the update. Please check the pending comments and also update the conditionals on booleans variables to match the if structure in line 88 (check for 1 OR yes|true):
if [[ "$bool" = 1 || "$bool" =~ ^(yes|true)$ ]]: then
    ...
fi

@gongomgra The new one has been submitted and supports BITNAMI_COLOR=1. I would like to explain that for simple and short logic, using && or || may be more concise, and omitting if can also improve readability. If complex logic is generated later, I will re-use if to write

huangjc7 · 2025-02-06T09:12:48Z

@gongomgra All the modified contents have been submitted in the final version. Please review them. Thank you.

gongomgra

Thanks for the update! I have added new comments, and there are still some pending comments from previous reviews. Can you please check all of them?

gongomgra · 2025-02-11T09:02:04Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

-    if ! [[ "$bool" = 1 || "$bool" =~ ^(yes|true)$ ]]; then
-        printf "%b\\n" "${*}" >&2
-    fi
+    ! [[ "$bool" = 1 || "$bool" =~ ^(yes|true)$ ]] && printf "%b\\n" "${*}" >&2


It may be that we are more used to the previous conditional structure than this one, but please revert this change for all occurrences.

gongomgra · 2025-02-11T09:04:55Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

+    else
+        prefix="${MODULE:-} $(date "+%T.%2N ")"
+    fi
+    stderr_print "${prefix}${*}${suffix}"


I think the suffix variable is not necessary here. We can simply use ${RESET} in stderr_print and avoid allocating memory for it (even if it is almost free). Please consider this review for the rest of the functions with the same structure.

gongomgra · 2025-02-11T09:08:21Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

+        reset_color=""
+    fi
+    log "${msg_color}ERROR${reset_color} ==> ${*}"
+    exit 1


Do not disregard this comment, please.

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

huangjc7 · 2025-02-12T03:46:40Z

@gongomgra New changes have been submitted, please check
Some questions that need to be discussed have also been submitted

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

gongomgra

Thank you for applying all the feedback! It is only pending to remove the errorX() method. The rest look good to me

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

huangjc7 · 2025-02-19T09:30:50Z

@gongomgra Submitted, please check

gongomgra

Sorry, one of my teammates noticed another minor change. Could you implement it for all your updated methods?

gongomgra · 2025-02-21T17:11:11Z

bitnami/redis-cluster/7.4/debian-12/prebuildfs/opt/bitnami/scripts/liblog.sh

-    stderr_print "${CYAN}${MODULE:-} ${MAGENTA}$(date "+%T.%2N ")${RESET}${*}"
+    local prefix=""
+    local color_bool="${BITNAMI_COLOR:-true}"
+    if [[ "$color_bool" = 1 || "$color_bool" =~ ^(yes|true)$ ]]; then


Let's include the additional options for the checks that are already present in other parts of the file

shopt -s nocasematch

huangjc7 · 2025-02-22T06:58:18Z

Sorry, one of my teammates noticed another minor change. Could you implement it for all your updated methods?

@gongomgra shopt -s nocasematch is effective globally, and I did not find any problems in my test.

gongomgra

Thanks for your contribution!

Added errorX function, which exits with non-zero value after execution

3415c61

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

github-actions bot added redis-cluster triage Triage is needed labels Jan 14, 2025

github-actions bot assigned javsalgar Jan 14, 2025

github-actions bot requested a review from javsalgar January 14, 2025 16:18

carrodher added verify Execute verification workflow for these changes in-progress labels Jan 14, 2025

github-actions bot removed the triage Triage is needed label Jan 14, 2025

github-actions bot unassigned javsalgar Jan 14, 2025

github-actions bot removed the request for review from javsalgar January 14, 2025 17:09

github-actions bot assigned gongomgra Jan 14, 2025

github-actions bot requested a review from gongomgra January 14, 2025 17:09

gongomgra requested changes Jan 21, 2025

View reviewed changes

Added errorX function, which exits with non-zero value after execution

6b00154

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

gongomgra requested changes Jan 28, 2025

View reviewed changes

Added errorX function, which exits with non-zero value after execution

cb6719d

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

huangjc7 requested a review from gongomgra February 2, 2025 09:55

gongomgra requested changes Feb 11, 2025

View reviewed changes

Added errorX function, which exits with non-zero value after execution

c90db4c

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

Added errorX function, which exits with non-zero value after execution

fc3468e

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

gongomgra requested changes Feb 19, 2025

View reviewed changes

Added errorX function, which exits with non-zero value after execution

eb9ca95

Added BITNAMI_COLOR method, which can control the color switch Signed-off-by: huangjc7 <[email protected]>

gongomgra requested changes Feb 21, 2025

View reviewed changes

gongomgra approved these changes Feb 27, 2025

View reviewed changes

gongomgra merged commit e952c9d into bitnami:main Feb 27, 2025
11 checks passed

github-actions bot added solved and removed in-progress labels Feb 27, 2025

		local color_check="${BITNAMI_COLOR:-true}"
		if ! [[ "$color_check" =~ ^(yes\|true)$ ]]; then

Some functions of liblog.sh log component are enhanced #76245

Some functions of liblog.sh log component are enhanced #76245

Uh oh!

Conversation

huangjc7 commented Jan 14, 2025

Description of the change

Uh oh!

huangjc7 commented Jan 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gongomgra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gongomgra left a comment

Choose a reason for hiding this comment

Uh oh!

huangjc7 commented Jan 29, 2025

Uh oh!

huangjc7 commented Feb 6, 2025

Uh oh!

gongomgra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huangjc7 commented Feb 12, 2025

Uh oh!

gongomgra left a comment

Choose a reason for hiding this comment

Uh oh!

huangjc7 commented Feb 19, 2025

Uh oh!

gongomgra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

huangjc7 commented Feb 22, 2025

Uh oh!

gongomgra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

huangjc7 commented Jan 15, 2025 •

edited

Loading