Skip to content

[chore] Pull systemd service logs on package test failure #860

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

Merged
merged 2 commits into from
Mar 20, 2025

Conversation

douglascamata
Copy link
Member

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
Member 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
Member 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
Member 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
Member Author

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

@douglascamata
Copy link
Member 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.

@jackgopack4
Copy link
Contributor

I think he means separate PR; just makes it easier to review.

@douglascamata douglascamata force-pushed the improve-package-tests-logs branch from 18e67f4 to 3093602 Compare March 19, 2025 15:52
@douglascamata
Copy link
Member Author

@jackgopack4 done.

Copy link
Contributor

@jackgopack4 jackgopack4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for splitting out the linter changes

@mx-psi mx-psi added this pull request to the merge queue Mar 20, 2025
Merged via the queue into open-telemetry:main with commit 7b544e1 Mar 20, 2025
70 checks passed
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.

5 participants