Skip to content
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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

douglascamata
Copy link
Contributor

This should help us identify issues introduced in the release build process related to RPM/deb packaging.

@douglascamata douglascamata requested a review from a team as a code owner March 6, 2025 10:09
Copy link
Member

@mowies mowies left a 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? 🤔

@douglascamata
Copy link
Contributor Author

@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.

@douglascamata
Copy link
Contributor Author

@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:

╰─ ./test.sh
stacked first
stacked second
my other echo
my echo

@douglascamata
Copy link
Contributor Author

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.

@mowies
Copy link
Member

mowies commented Mar 6, 2025

sounds good to me :)

@atoulme
Copy link
Contributor

atoulme commented Mar 6, 2025

Can we make the whitespace changes separate? Can you approve the PR if you're ok with it @mowies ?

@douglascamata
Copy link
Contributor Author

@atoulme you mean in a separate PR? The formatting is already separated in its own commit.

@douglascamata
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants