Re: [XEN PATCH] automation/eclair: Make report browsing URL configurable.

[Anthony PERARD <anthony@xxxxxxxxxxxxxx>]

On Fri, Jun 27, 2025 at 10:39:21AM +0200, Nicola Vetrini wrote:
> On 2025-06-26 12:08, Anthony PERARD wrote:
> > On Thu, Jun 26, 2025 at 08:38:18AM +0200, Nicola Vetrini wrote:
> > > +eclairReportHost="${MACHINE_HOST:-saas.eclairit.com}"
> > > +eclairReportPort="${MACHINE_PORT:-3787}"
> > > +eclairReportUrlPrefix="https://${eclairReportHost}:${eclairReportPort}";
> > 
> > Please, don't make the port number mandatory. Can you merge both host
> > and port in the same variable? This part seems to be called "authority":
> > 
> >     https://en.wikipedia.org/wiki/URL#Syntax
> > 
> > Also, don't use `MACHINE` as prefix/namespace for these new variables,
> > in a pipeline context, "machine" could be many things. Maybe
> > "ECLAIR_REPORT_HOST" for this one? With the default been:
> > 
> >     ECLAIR_REPORT_HOST=saas.eclairit.com:3787
> > 
> 
> I can merge host and port and change the variable prefix, but I think there
> is a misunderstanding. This address is used both as the base for report
> browsing and pushing the results.

Well, the patch is all about "report browsing URL" as stated in the
subject.

> While we should alter the latter (e.g.,
> ECLAIR_REPORT_PROXY_HOST) to point to the proxy so that the proxy is shown

"proxy"? That's implementation detail, there's no need to know that the
host used to browse the result is different than the host where the
result are stored. (I mean having "proxy" in the name of the variable
holding detail of how to access the reports, been a part of an url or
just the host)

> in the CI logs, the address where the results are pushed is fixed and set in
> the docker runner env. This is not ideal, but I didn't find a better way
> with GitLab CI to let the analysis push locally.

Having the configuration for where to push the result in the runner env
seems like the best option to me, if that report server is tied to the
eclair runner.

> > > diff --git a/automation/scripts/eclair b/automation/scripts/eclair
> > > index 0a2353c20a92..7020eaa0982f 100755
> > > --- a/automation/scripts/eclair
> > > +++ b/automation/scripts/eclair
> > > @@ -1,4 +1,15 @@
> > > -#!/bin/sh -eu
> > > +#!/bin/sh -eux
> > > +
> > > +# Runner-specific variables
> > > +ex=0
> > > +export "$(env | grep MACHINE_ARTIFACTS_ROOT)" || ex=$?
> > > +[ "${ex}" = 0 ] || exit "${ex}"
> > 
> > That's a really complicated way to check a variable is set...
> > Exporting a variable that's already in env isn't useful, and I think
> > `ex` is only ever set to `0`. It seems that `dash` just exit if you do
> > `export=""`.
> > 
> > You could simply do:
> > 
> >     : ${MACHINE_ARTIFACTS_ROOT:?Missing MACHINE_ARTIFACTS_ROOT variable}
> >     : ${MACHINE_ECDF_DIR:?Missing MACHINE_ECDF_DIR variable}
> > 
> > To check that the variables are set. Or nothing, if you add `set -u` to
> > the script (instead of the one -u in the sheband which might be ignored
> > if one run `sh ./eclair` instead of just `./eclair`.) Also the variable
> > should come from the env, as nothing sets it, so no need to for that.
> > 
> > ( The `:` at the begining of the line is necessary, and behave the same
> > way as `true` does. We need it because ${parm:?msg} is expanded.)
> > 
> > Or you could use `if [ -z "${param}" ]` if ${param:?msg} is too obscure.
> > We would just have "parameter not set" instead of a nicer message, due
> > to `set -u`.
> > 
> 
> I agree it is ugly and counterintuitive, but the core idea here is that the
> variable is set but not exported for some reason, so just `export VAR` does
> not behave in the same way as the incantation `export "$(env | grep
> MACHINE_ARTIFACTS_ROOT)"` iirc. I'll double check if there's a better way to
> achieve this (other than switching to bash in the shebang).

Isn't that script `./eclair` the beginning of the script? I can't find
anything that `source` this file so it must be. Which mean, if
MACHINE_ARTIFACTS_ROOT is set, it is exported, because nothing sets it
between the beginning of the script and this line. So there's no need to
check that the variable is exported (if it is not exported, it is a bug
in the shell).

The only thing left to do is to check that it is set and non-zero
length. But I still wonder why you do that here, since "action.settings"
also check the variable and use a default value.

There's no need to use bash for this. (They are plenty of reason to use
bash instead of posix shell, but that is just not one of them.)

Cheers,

-- 
Anthony PERARD