[LTP] [PATCH v5 2/4] ci: add patchwork communication script
Petr Vorel
pvorel@suse.cz
Mon Apr 14 16:02:57 CEST 2025
Hi Andrea,
Generally LGTM, few notes below.
Feel free to ignore them, note at '-e' and '-z "$run" -o' (at the end) are bugs
worth to be fixed.
> Add a script to communicate with patchwork. Available commands are:
> - state: change patch-series state
> - check: send a tests report to patchwork
> - verify: will print a list of new patch-series which has not been
> tested in the past hour (by default)
> The script can be configured defining:
> - PATCHWORK_URL: patchwork url to communicate with
> - PATCHWORK_TOKEN: patchwork authentication token
> - PATCHWORK_SINCE: timespan in seconds where we want to fetch
> patch-series
> Signed-off-by: Andrea Cervesato <andrea.cervesato@suse.com>
> ---
> ci/tools/patchwork.sh | 197 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 197 insertions(+)
> create mode 100755 ci/tools/patchwork.sh
> diff --git a/ci/tools/patchwork.sh b/ci/tools/patchwork.sh
> new file mode 100755
> index 000000000..d43b7fd61
> --- /dev/null
> +++ b/ci/tools/patchwork.sh
+1 for having the script in tools directory (the ci/ directory is kept for
distro installation scripts, which can be reused by the others).
> @@ -0,0 +1,197 @@
> +#!/bin/sh -ex
-e in shebang (i.e. 'set -e') means quit after error.
This will lead to some unreachable code. If you have proper error handling, '-e'
should not be needed. Or if you want really to '-e' cause to quit early, please
drop whole error handling (it's useless). '-x' is quite verbose, it will be
visible when the code quit (just without your explanations).
see "Argument List Processing"
https://man7.org/linux/man-pages/man1/dash.1.html
or
https://www.gnu.org/software/bash/manual/bash.html#Bourne-Shell-Builtins#The-Set-Builtin-1
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Shell script that can be used to communicate with Patchwork via REST API.
> +# It has been mainly created for CI purposes, but it can be used in the shell
> +# by satisfing minimum requirements.
typo: s/satisfing/satisfying/
> +#
> +# Copyright (c) 2025 Andrea Cervesato <andrea.cervesato@suse.com>
> +
> +command_exists() {
> + command -v $1 >/dev/null 2>&1
Here script quit if $1 will not be found...
> +
> + if [ $? -ne 0 ]; then
> + echo "'$1' must be present in the system"
nit: maybe redirect error to stderr?
echo "'$1' must be present in the system" >&2
> + exit 1
> + fi
... Therefore the above code will not be shown when shebang contains '-e'.
> +}
> +
> +command_exists "curl"
> +command_exists "jq"
very minor: I would make a loop in command_exists (as done in tst_require_cmds) and call it:
command_exists curl jq
> +
> +if [ -z "$PATCHWORK_URL" ]; then
> + PATCHWORK_URL="https://patchwork.ozlabs.org"
> +fi
> +
> +if [ -z "$PATCHWORK_SINCE" ]; then
> + PATCHWORK_SINCE=3600
> +fi
IMHO it's more readable if global variables are defined above the functions
(i.e. before command_exists(), not in the middle of function setup.
Also all shells support :- with default parameter:
PATCHWORK_URL="${PATCHWORK_URL:-https://patchwork.ozlabs.org}"
PATCHWORK_SINCE="${PATCHWORK_SINCE:-3600}"
> +
> +fetch_series() {
> + local current_time=$(date +%s)
> + local since_time=$(expr $current_time - $PATCHWORK_SINCE)
> + local date=$(date -u -d @$since_time +"%Y-%m-%dT%H:%M:%SZ")
> +
> + curl -k -G "$PATCHWORK_URL/api/events/" \
> + --data "category=series-completed" \
> + --data "project=ltp" \
> + --data "state=new" \
> + --data "since=$date" \
> + --data "archive=no" |
> + jq -r '.[] | "\(.payload.series.id):\(.payload.series.mbox)"'
> +
> + if [ $? -ne 0 ]; then
> + exit 1
Again, '-e' means quit immediately after curl exit with non-zero code.
Only if you decide to remove '-e': curl has various exit codes. I was thinking
if exit the real curl exit code would be useful. OTOH the error message will be
visible, thus probably not needed.
> + fi
> +}
> +
> +get_patches() {
> + local series_id="$1"
> +
> + curl -k -G "$PATCHWORK_URL/api/patches/" \
> + --data "project=ltp" \
> + --data "series=$series_id" |
> + jq -r '.[] | "\(.id)"'
> +
> + if [ $? -ne 0 ]; then
> + exit 1
> + fi
> +}
> +
> +verify_token_exists() {
> + if [ -z "$PATCHWORK_TOKEN" ]; then
> + echo "For this feature you need \$PATCHWORK_TOKEN"
> + exit 1
> + fi
> +}
> +
> +set_patch_state() {
> + local patch_id="$1"
> + local state="$2"
> +
> + verify_token_exists
> +
> + curl -k -X PATCH \
> + -H "Authorization: Token $PATCHWORK_TOKEN" \
> + -F "state=$state" \
> + "$PATCHWORK_URL/api/patches/$patch_id/"
> +
> + if [ $? -ne 0 ]; then
> + exit 1
> + fi
> +}
> +
> +set_series_state() {
> + local series_id="$1"
> + local state="$2"
> +
> + get_patches "$series_id" | while IFS= read -r patch_id; do
> + if [ -n "$patch_id" ]; then
> + set_patch_state "$patch_id" "$state"
> + fi
> + done
> +}
> +
> +get_checks() {
> + local patch_id="$1"
> +
> + curl -k -G "$PATCHWORK_URL/api/patches/$patch_id/checks/" |
> + jq -r '.[] | "\(.id)"'
> +
> + if [ $? -ne 0 ]; then
> + exit 1
> + fi
> +}
> +
> +already_tested() {
> + local series_id="$1"
> +
> + get_patches "$series_id" | while IFS= read -r patch_id; do
> + if [ ! -n "$patch_id" ]; then
> + continue
> + fi
FYI: common shell shorter (works everywhere):
[ -n "$patch_id" ] || continue
Or even:
[ "$patch_id" ] || continue
> +
> + get_checks "$patch_id" | while IFS= read -r check_id; do
> + if [ -n "$check_id" ]; then
> + echo "$check_id"
> + return
> + fi
> + done
> + done
> +}
> +
> +verify_new_patches() {
> + local output="series_ids.txt"
> +
> + echo -n '' >"$output"
series_ids.txt is stored in current working directory, right? Clearer it'd be to
use mktemp (part of coreutils and busybox, probably as common as date, which we
also expect to be available, at least it's on Alpine).
That way git clone is not polluted.
> +
> + fetch_series | while IFS=: read -r series_id series_mbox; do
> + if [ ! -n "$series_id" ]; then
> + continue
> + fi
> +
> + tested=$(already_tested "$series_id")
> + if [ -n "$tested" ]; then
> + continue
> + fi
> +
> + echo "$series_id|$series_mbox" >>"$output"
> + done
> +
> + cat "$output"
> +}
> +
> +send_results() {
> + local series_id="$1"
> + local target_url="$2"
> +
> + verify_token_exists
> +
> + local context=$(echo "$3" | sed 's/:/_/g; s/\//-/g; s/\./-/g')
> + if [ -n "$CC" ]; then
> + context="${context}_${CC}"
> + fi
> +
> + if [ -n "$ARCH" ]; then
> + context="${context}_${ARCH}"
> + fi
> +
> + local result="$4"
> + if [ "$result" = "cancelled" ]; then
> + return
> + fi
> +
> + local state="fail"
> + if [ "$result" = "success" ]; then
> + state="success"
> + fi
> +
> + get_patches "$series_id" | while IFS= read -r patch_id; do
> + if [ -n "$patch_id" ]; then
> + curl -k -X POST \
> + -H "Authorization: Token $PATCHWORK_TOKEN" \
> + -F "state=$state" \
> + -F "context=$context" \
> + -F "target_url=$target_url" \
> + -F "description=$result" \
> + "$PATCHWORK_URL/api/patches/$patch_id/checks/"
This will help you to save one indent:
[ -z "$patch_id" ] || continue
curl -k -X POST \
> +
> + if [ $? -ne 0 ]; then
> + exit 1
> + fi
> + fi
> + done
> +}
> +
> +run="$1"
> +
> +if [ -z "$run" -o "$run" = "state" ]; then
> + set_series_state "$2" "$3"
> +elif [ -z "$run" -o "$run" = "check" ]; then
'-z "$run"' is unreachable code (if $run is empty, it's run in the above if and
both elif and else blocks are skipped).
You could have something like:
case "$1" in
state|check|verify|"") run="$1";;
*) echo "Available commands: state, check, verify"; exit 1;;
esac
if [ -z "$run" -o "$run" = "state" ]; then
set_series_state "$2" "$3"
fi
if [ -z "$run" -o "$run" = "check" ]; then
send_results "$2" "$3" "$4" "$5"
fi
if [ -z "$run" -o "$run" = "verify" ]; then
verify_new_patches
fi
Kind regards,
Petr
> + send_results "$2" "$3" "$4" "$5"
> +elif [ -z "$run" -o "$run" = "verify" ]; then
> + verify_new_patches
> +else
> + echo "Available commands: state, check, verify"
> + exit 1
> +fi
More information about the ltp
mailing list