Re: [PATCH] automation: fix eclair scanning for merge requests

[Simone Ballarin <simone.ballarin@xxxxxxxxxxx>]

On 2024-08-06 03:03, Stefano Stabellini wrote:
> On Sat, 3 Aug 2024, Nicola Vetrini wrote:
> > On 2024-08-02 00:06, Stefano Stabellini wrote:
> > > The ECLAIR jobs part of the Gitlab CI pipeline fail reliably when the
> > > pipeline is started from a merge request. This is the error:
> > >
> > > Unexpected event pull_request
> > >
> > > The error is a consequence of action.settings setting event=pull_request
> > > for merge_request_event. Given that we don't need any special behavior
> > > from ECLAIR for merge requests, just run the regular scan, change
> > > action.settings to set event=push for merge_request_event (same as for
> > > pipeline triggered by git push).
> > >
> >
> > According to my analysis, the error stems from the fact that
> > automation/scripts/eclair unconditionally calls action_push.sh, which 
> > is not
> > designed to handle merge requests (that would be 
> > action_pull_request.sh). One
> > approach, that needs to be tested, could be to execute that second 
> > script
> > based on the value of ${CI_PIPELINE_SOURCE}.
> >
> > The main difference the approach taken in this patch is that the 
> > analysis
> > results will be relative to the nearest merge point with the staging 
> > tree. If
> > that's not something that is desired, then this approach looks good to 
> > me.
> You are right. I tested the below and it works as expected, see:
> https://gitlab.com/xen-project/people/sstabellini/xen/-/pipelines/1401863307
Hi Stefano,

the patch looks ok. Don't know whether is should be submitted again. 
Only one minor nit:
> ---
> From dee036081cb367862daba264e1eb84ca2505f678 Mon Sep 17 00:00:00 2001
> From: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> Date: Mon, 5 Aug 2024 14:24:23 -0700
> Subject: [PATCH] automation: fix eclair gitlab jobs for merge requests
>
> The "eclair" script calls action_push.sh even for merge request, while
> instead action_pull_request.sh should be called, resulting in a job
> failure with this error:
>
> Unexpected event pull_request
>
> Fix the script to call action_pull_request.sh appropriately.
>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxx>
> ---
>  automation/eclair_analysis/ECLAIR/action_pull_request.sh | 2 +-
>  automation/scripts/eclair                                | 9 ++++++++-
>  2 files changed, 9 insertions(+), 2 deletions(-)
>  mode change 100644 => 100755 
> automation/eclair_analysis/ECLAIR/action_pull_request.sh
> diff --git a/automation/eclair_analysis/ECLAIR/action_pull_request.sh 
> b/automation/eclair_analysis/ECLAIR/action_pull_request.sh
> old mode 100644
> new mode 100755
> index 68f7e6282e..812238e07f
> --- a/automation/eclair_analysis/ECLAIR/action_pull_request.sh
> +++ b/automation/eclair_analysis/ECLAIR/action_pull_request.sh
> @@ -3,7 +3,7 @@
>  set -eu
>
>  usage() {
> -    echo "Usage: $0 WTOKEN ANALYSIS_OUTPUT_DIR COMMIT_ID" >&2
> +    echo "Usage: $0 WTOKEN ANALYSIS_OUTPUT_DIR" >&2
>      exit 2
>  }
>
> diff --git a/automation/scripts/eclair b/automation/scripts/eclair
> index ebcaf09b54..97d743ab53 100755
> --- a/automation/scripts/eclair
> +++ b/automation/scripts/eclair
> @@ -21,5 +21,12 @@ ex=0
>  # Fail in case of new reports for clean guidelines
>  "${ECLAIR_DIR}/action_check_clean_regressions.sh" 
> "${ECLAIR_OUTPUT_DIR}" || ex=$?
> -"${ECLAIR_DIR}/action_push.sh" "${WTOKEN}" "${ECLAIR_OUTPUT_DIR}"
> +case "${CI_PIPELINE_SOURCE}" in
> +merge_request_event)
> +    "${ECLAIR_DIR}/action_pull_request.sh" "${WTOKEN}" 
> "${ECLAIR_OUTPUT_DIR}"
> +    ;;
> +push | pipeline | web | schedule)
> +    "${ECLAIR_DIR}/action_push.sh" "${WTOKEN}" "${ECLAIR_OUTPUT_DIR}"
> +    ;;
> +esac
here I think it's better to give an error if the event is not handled. 
Something similar to
*)
    echo "Unexpected event ${CI_PIPELINE_SOURCE}" >&2
    ex=1
    ;;


>  [ "${ex}" = 0 ] || exit "${ex}"