-
Notifications
You must be signed in to change notification settings - Fork 179
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
[chore] Pull systemd service logs on package test failure #860
base: main
Are you sure you want to change the base?
[chore] Pull systemd service logs on package test failure #860
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does having 2 traps work? 🤔
@mowies good catch! I just learned that we can only have one trap per signal. I think I found a workaround that is a nice: creating a trap to call functions from a list, then append more functions to the list as necessary. Let me see runs some tests and see if it'll work. |
@mowies I pushed a fix with an approach to allow us to stack multiple functions to be "trapped". Let me know what you think. Tested locally with the script: #!/bin/bash
# Copyright The OpenTelemetry Authors
# SPDX-License-Identifier: Apache-2.0
set -euo pipefail
TRAP_FUNCS=()
add_trap_func() {
TRAP_FUNCS+=("$1")
}
run_traps() {
if [ "${#TRAP_FUNCS[@]}" -gt 0 ]; then
for ((i = ${#TRAP_FUNCS[@]} - 1; i >= 0; i--)); do
"${TRAP_FUNCS[i]}"
done
fi
}
trap 'run_traps' EXIT
my_echo() {
echo "my echo"
}
my_other_echo() {
echo "my other echo"
}
add_trap_func my_echo
echo "stacked first"
add_trap_func my_other_echo
echo "stacked second" And got:
|
A nice "plus" of this approach is that it only works if you define the traps as functions, which allows linting tools like shellcheck and others to run and boost confidence on the code. Previously the trapped code was inside a string that was treated just like any random string and could hide bugs until runtime. |
sounds good to me :) |
Can we make the whitespace changes separate? Can you approve the PR if you're ok with it @mowies ? |
@atoulme you mean in a separate PR? The formatting is already separated in its own commit. |
Hey @atoulme, can you check my previous reply regarding the commit with the whitespaces change? Just want to clarify whether "making the whitespace changes separate" means a separate PR or commit, because it's already in a separate commit. |
This should help us identify issues introduced in the release build process related to RPM/deb packaging.