From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ian Campbell <ian.campbell@citrix.com>
Subject: Re: [PATCH OSSTEST 2/5] Handle osstest's own local push
 gate in non-master production instances
Date: Thu, 2 Apr 2015 12:21:35 +0100
Message-ID: <1427973695.4037.54.camel@citrix.com>
References: <1427900998.13425.21.camel@citrix.com>
	<1427901022-24314-2-git-send-email-ian.campbell@citrix.com>
	<21789.9281.449911.318689@mariner.uk.xensource.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <21789.9281.449911.318689@mariner.uk.xensource.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Ian Jackson <Ian.Jackson@eu.citrix.com>
Cc: xen-devel@lists.xen.org
List-Id: xen-devel@lists.xenproject.org

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.