fix(RELEASE-1108): improve error messaging

[scoheb]

• capture the error message when validating.

[johnbieren]

Why doesn't this work as is? jq -e is supposed to fail if the key isn't found

[scoheb]

Why doesn't this work as is? jq -e is supposed to fail if the key isn't found

Yes. 'jq -e' does do that...but all that gets transferred via the trap would be "exit 1" as the error message.

This way we detect the error silently via the " == null " and then write a nice error message to the stderr file and then exit.

The contents is the stderr file is what is used to relay back to the user in the release pipeline and in this case is a better UX

[johnbieren]

Why doesn't this work as is? jq -e is supposed to fail if the key isn't found

Yes. 'jq -e' does do that...but all that gets transferred via the trap would be "exit 1" as the error message.

This way we detect the error silently via the " == null " and then write a nice error message to the stderr file and then exit.

The contents is the stderr file is what is used to relay back to the user in the release pipeline and in this case is a better UX

Wouldn't the existing code do that with just the addition of | tee like is shown in the new version? The echo was already there

[johnbieren]

It works great here https://github.com/hacbs-release/app-interface-deployments/blob/main/internal-services/catalog/pulp-push-disk-images-task.yaml#L163 so I am not sure why we need a new style with additional if checks here

[scoheb]

It works great here https://github.com/hacbs-release/app-interface-deployments/blob/main/internal-services/catalog/pulp-push-disk-images-task.yaml#L163 so I am not sure why we need a new style with additional if checks here

When I tested this, it did not work.

I can show you my test script and it's output later...

[johnbieren]

The reason why it doesn't work isn't the -e. It is because this https://github.com/hacbs-release/app-interface-deployments/blob/main/internal-services/catalog/pulp-push-disk-images-task.yaml#L271 is not sent to the STDERR_FILE. You need to either do that or to add a | tee in the current echoes like you do in this PR

[scoheb]

Consider this script that mimics process_component() before this PR:

#!/usr/bin/env bash
set -ex

STDERR_FILE=/tmp/stderr.txt
RESULT_FILE=/tmp/results.txt
rm -f $STDERR_FILE $RESULT_FILE

exitfunc() {
    local err=$1
    local line=$2
    local command="$3"
    if [ "$err" -eq 0 ] ; then
        echo -n "Success" > "$RESULT_FILE"
    else
        echo "$0: ERROR '$command' failed at line $line - exited with status $err" \
          > "$RESULT_FILE"
        if [ -f "$STDERR_FILE" ] ; then
            cat "$STDERR_FILE" | tail -n 20 >> "$RESULT_FILE"
        fi
    fi
    exit 0 # exit the script cleanly as there is no point in proceeding past an error or exit call
}
# due to set -e, this catches all EXIT and ERR calls and the task should never fail with nonzero exit code
trap 'exitfunc $? $LINENO "$BASH_COMMAND"' EXIT

process_component() {
  COMPONENT="{}"
  sleep 10
  DESTINATION="$(jq -er '.staged.destination' <<< "${COMPONENT}")" \
    || (echo Missing staged.destination value for component. This should be an existing pulp repo. Failing \
    && exit 1)
}

RUNNING_JOBS="\j" # Bash parameter for number of jobs currently running
process_component & 2> $STDERR_FILE

while (( ${RUNNING_JOBS@P} > 0 )); do
    echo "waiting: RUNNING_JOBS = ${RUNNING_JOBS@P}"
    wait -n
done

when it executes, /tmp/stderr.txt is empty

We'd have to amend it with a tee -a to be:

  DESTINATION="$(jq -er '.staged.destination' <<< "${COMPONENT}")" \
    || (echo 'Missing staged.destination value for component. This should be an existing pulp repo. Failing' | tee -a $STDERR_FILE \
    && exit 1)

[johnbieren]

Consider this script that mimics process_component() before this PR:

#!/usr/bin/env bash
set -ex

STDERR_FILE=/tmp/stderr.txt
RESULT_FILE=/tmp/results.txt
rm -f $STDERR_FILE $RESULT_FILE

exitfunc() {
    local err=$1
    local line=$2
    local command="$3"
    if [ "$err" -eq 0 ] ; then
        echo -n "Success" > "$RESULT_FILE"
    else
        echo "$0: ERROR '$command' failed at line $line - exited with status $err" \
          > "$RESULT_FILE"
        if [ -f "$STDERR_FILE" ] ; then
            cat "$STDERR_FILE" | tail -n 20 >> "$RESULT_FILE"
        fi
    fi
    exit 0 # exit the script cleanly as there is no point in proceeding past an error or exit call
}
# due to set -e, this catches all EXIT and ERR calls and the task should never fail with nonzero exit code
trap 'exitfunc $? $LINENO "$BASH_COMMAND"' EXIT

process_component() {
  COMPONENT="{}"
  sleep 10
  DESTINATION="$(jq -er '.staged.destination' <<< "${COMPONENT}")" \
    || (echo Missing staged.destination value for component. This should be an existing pulp repo. Failing \
    && exit 1)
}

RUNNING_JOBS="\j" # Bash parameter for number of jobs currently running
process_component & 2> $STDERR_FILE

while (( ${RUNNING_JOBS@P} > 0 )); do
    echo "waiting: RUNNING_JOBS = ${RUNNING_JOBS@P}"
    wait -n
done

when it executes, /tmp/stderr.txt is empty

We'd have to amend it with a tee -a to be:

  DESTINATION="$(jq -er '.staged.destination' <<< "${COMPONENT}")" \
    || (echo 'Missing staged.destination value for component. This should be an existing pulp repo. Failing' | tee -a $STDERR_FILE \
    && exit 1)

I am not following. process_component isn't changed at all by this PR. process_component_for_developer_portal is. But in the snippet, you are showing all process_component code. Second, you shouldn't be checking /tmp/stderr.txt, the file to check is /tmp/results.txt. Finally, you moved the & in the script you pasted. The code is process_component "$COMPONENT" 2> "$STDERR_FILE" &. You wrote process_component & 2> $STDERR_FILE. If I copy your mimic script and change that line to what it is in the task,

$ cat /tmp/results.txt 
test: ERROR 'wait -n' failed at line 1 - exited with status 1
+ COMPONENT='{}'
+ sleep 10
++ jq -er .staged.destination
+ DESTINATION=null
+ echo Missing staged.destination value for component. This should be an existing pulp repo. Failing
+ exit 1

[scoheb]

/retest

[scoheb]

Thanks for setting me straight. I was initially swayed with the fact that the python script were not logging to stdout.

I think this looks better now