Re: [PATCH OSSTEST 2/5] Handle osstest's own local push gate in non-master production instances

[Ian Campbell <ian.campbell@citrix.com>]

On Thu, 2015-04-02 at 12:13 +0100, Ian Jackson wrote:
> Ian Campbell writes ("[PATCH OSSTEST 2/5] Handle osstest's own local push gate in non-master production instances"):
> > We want to arrange that the master XenProject instance continues to
> > test its own pretest branch while any downstream instances will pickup
> > changes from the master instance's production (i.e. tested) branch,
> > which is published at git://xenbits.xen.org/osstest.git#master.
> ...	;;
> >  osstest)
> > -	if [ "x$OSSTEST_USE_HEAD" != "xy" ] ; then
> > +	if [ x"$TREEBRANCH_OSSTEST_UPSTREAM" != x ] ; then
> 
> Shouldn't the OSSTEST_USE_HEAD option override this ?  Ie I think this
> is the wrong way round.

Yes, I think you are right.

I think I just ended up with this because I was testing via the
standalone wrapper which sets USE_HEAD, but I'll test another way.

> > +	    OSSTEST_REVISION_MERGE=`repo_tree_rev_fetch_git osstest \
> > +		$TREEBRANCH_OSSTEST_UPSTREAM $LOCALREV_OVMF`
> 
> OSSTEST_REVISION_MERGE doesn't seem to be an environment variable.  I
> think it should either be honoured if pre-set, or just be an ordinary
> (lowercase) variable name (in which case it doesn't need the OSSTEST
> prefix).

OK. Any preference as to which?

As it stands the only reason for the variable is to sink the output from
repo_tree_rev_fetch and log it.

> 
> > +	    echo >&2 "$TREEBRANCH_OSSTEST_UPSTREAM = $OSSTEST_REVISION_MERGE"
> > +
> > +	    rm -rf $repos/osstest-merge >&2
> > +	    git clone -b pretest $HOME/testing.git $repos/osstest-merge >&2
> > +
> > +	    git -C $repos/osstest-merge \
> > +		fetch $repos/osstest $LOCALREV_OVMF:ap-merge >&2
> 
> LOCALREV_OVMF (here and earlier).  It's not clear to me that you
> actually need to indirect it; you could just specify a branch name.

OK.

> The rest looks fine.
> 
> Having thought about how to override things, particularly:
> 
> > +	if [ x"$TREEBRANCH_OSSTEST_UPSTREAM" != x ] ; then
> > +	    # could push to instance specific location, but
> > +	    # certainly not to master instance's xenbits repo!
> > +	    :
> > +	else
> 
> I think ap-fetch-* and ap-push should have a general purpose hook
> which occurs just after ap-common is loaded, something like this:
> 
>   diff --git a/ap-fetch-version b/ap-fetch-version
>   index 33aaf00..a3028f6 100755
>   --- a/ap-fetch-version
>   +++ b/ap-fetch-version
>   @@ -31,6 +31,8 @@ if info_linux_tree "$branch"; then
>           exit 0
>    fi
> 
>   +. ${OSSTEST_HOOK_AP_FETCH-/dev/null}
>   +
>    case "$branch" in
>    xen-3.*)
>           ./sg-hg-heads sh -ec '
> 
> We can postpone actually writing that until someone needs it.

Gladly ;-)

Ian.