[PATCH 0/3] t/www_listing: update for Grokmirror v2

[PATCH 0/3] t/www_listing: update for Grokmirror v2

[Kyle Meyer]

The test suite is failing on my end due to incompatibilities with
Grokmirror v2:

  ERROR: Section [core] must exist in: /tmp/pi-www_listing-4290-4AZck2/repos.conf
         Perhaps this is a grokmirror-1.x config file?

Patch 3 switches the generated manifest to be compatible with v2,
skipping the tests for older Grokmirror versions.

  [1/3] t/www_listing: correct the number of tests for grok-pull skip
  [2/3] t/www_listing: reword grok-pull skip message
  [3/3] t/www_listing: require grok-pull version 2 or later

 t/www_listing.t | 36 +++++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 15 deletions(-)


base-commit: dda6a67ce8865641edad51f83aab2eb2fc27bda8
-- 
2.30.1

[PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip

[Kyle Meyer]

---

  I'm not positive that I'm understanding skip's $how_many argument or
  that it matters much.

 t/www_listing.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/www_listing.t b/t/www_listing.t
index 63ef2eacfd407d51..cdf2a1847a945225 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -121,7 +121,7 @@ SKIP: {
 	tiny_test($json, $host, $port);
 
 	my $grok_pull = which('grok-pull') or
-		skip('skipping grok-pull integration test', 2);
+		skip('skipping grok-pull integration test', 12);
 
 	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
 	open $fh, '>', "$tmpdir/repos.conf" or die;
-- 
2.30.1

Re: [PATCH 1/3] t/www_listing: correct the number of tests for grok-pull skip

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> ---
> 
>   I'm not positive that I'm understanding skip's $how_many argument or
>   that it matters much.

It doesn't seem to matter, anymore.

In the old days, Test::More required explicit plan test counts
for everything, but we've always relied on done_testing() (no
args) in our code base with no explicit plan.

I've given up maintaining counts inside test_lei; and
just using `1'.  I suppose it's safe to do for the rest
of our tests, but I'm not against this patch, either.

[PATCH 2/3] t/www_listing: reword grok-pull skip message

[Kyle Meyer]

Make it clear that this skip is because grok-pull isn't available at
all because the next commit will add another skip for older versions
of Grokmirror.
---
 t/www_listing.t | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/www_listing.t b/t/www_listing.t
index cdf2a1847a945225..bf35530f3494c016 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -121,7 +121,7 @@ SKIP: {
 	tiny_test($json, $host, $port);
 
 	my $grok_pull = which('grok-pull') or
-		skip('skipping grok-pull integration test', 12);
+		skip('grok-pull not available', 12);
 
 	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
 	open $fh, '>', "$tmpdir/repos.conf" or die;
-- 
2.30.1

[PATCH 3/3] t/www_listing: require grok-pull version 2 or later

[Kyle Meyer]

The grok-pull-based tests in www_listing are incompatible with
Grokmirror v2 in two ways: the generated configuration format and the
expected exit codes.  Update the tests to work with v2, and skip them
for earlier versions.

This was tested with the latest release of Grokmirror, v2.0.7.  Note
that the "pull" and "fsck" sections are required even though they're
empty.
---

  Another option would be to generate an appropriate v1 or v2
  configuration based on which Grokmirror version is detected.  I'm
  not sure that's worth the trouble though.

 t/www_listing.t | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/t/www_listing.t b/t/www_listing.t
index bf35530f3494c016..6a2892de9b827fe6 100644
--- a/t/www_listing.t
+++ b/t/www_listing.t
@@ -122,22 +122,27 @@ SKIP: {
 
 	my $grok_pull = which('grok-pull') or
 		skip('grok-pull not available', 12);
+	my ($grok_version) = (xqx([$grok_pull, "--version"])
+			=~ /(\d+)\.(?:\d+)(?:\.(\d+))?/);
+	$grok_version >= 2 or
+		skip('grok-pull v2 or later not available', 12);
 
 	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
 	open $fh, '>', "$tmpdir/repos.conf" or die;
 	print $fh <<"" or die;
-# You can pull from multiple grok mirrors, just create
-# a separate section for each mirror. The name can be anything.
-[test]
-site = http://$host:$port
-manifest = http://$host:$port/manifest.js.gz
+[core]
 toplevel = $tmpdir/mirror
-mymanifest = $tmpdir/local-manifest.js.gz
+manifest = $tmpdir/local-manifest.js.gz
+[remote]
+site = http://$host:$port
+manifest = \${site}/manifest.js.gz
+[pull]
+[fsck]
 
 	close $fh or die;
 
 	xsys($grok_pull, '-c', "$tmpdir/repos.conf");
-	is($? >> 8, 127, 'grok-pull exit code as expected');
+	is($? >> 8, 0, 'grok-pull exit code as expected');
 	for (qw(alt bare v2/git/0.git v2/git/1.git v2/git/2.git)) {
 		ok(-d "$tmpdir/mirror/$_", "grok-pull created $_");
 	}
@@ -146,18 +151,19 @@ mymanifest = $tmpdir/local-manifest.js.gz
 	# /$INBOX/v2/manifest.js.gz
 	open $fh, '>', "$tmpdir/per-inbox.conf" or die;
 	print $fh <<"" or die;
-# You can pull from multiple grok mirrors, just create
-# a separate section for each mirror. The name can be anything.
-[v2]
-site = http://$host:$port
-manifest = http://$host:$port/v2/manifest.js.gz
+[core]
 toplevel = $tmpdir/per-inbox
-mymanifest = $tmpdir/per-inbox-manifest.js.gz
+manifest = $tmpdir/per-inbox-manifest.js.gz
+[remote]
+site = http://$host:$port
+manifest = \${site}/v2/manifest.js.gz
+[pull]
+[fsck]
 
 	close $fh or die;
 	ok(mkdir("$tmpdir/per-inbox"), 'prepare single-v2-inbox mirror');
 	xsys($grok_pull, '-c', "$tmpdir/per-inbox.conf");
-	is($? >> 8, 127, 'grok-pull exit code as expected');
+	is($? >> 8, 0, 'grok-pull exit code as expected');
 	for (qw(v2/git/0.git v2/git/1.git v2/git/2.git)) {
 		ok(-d "$tmpdir/per-inbox/$_", "grok-pull created $_");
 	}
-- 
2.30.1

Re: [PATCH 3/3] t/www_listing: require grok-pull version 2 or later

[Eric Wong]

Kyle Meyer <kyle@kyleam.com> wrote:
> The grok-pull-based tests in www_listing are incompatible with
> Grokmirror v2 in two ways: the generated configuration format and the
> expected exit codes.  Update the tests to work with v2, and skip them
> for earlier versions.
> 
> This was tested with the latest release of Grokmirror, v2.0.7.  Note
> that the "pull" and "fsck" sections are required even though they're
> empty.
> ---
> 
>   Another option would be to generate an appropriate v1 or v2
>   configuration based on which Grokmirror version is detected.  I'm
>   not sure that's worth the trouble though.

Ugh, some of these incompatible changes to grokmirror are really
annoying and will break existing scripts when I upgrade.
(and I suspect this affects other people, too).

+Cc Konstantin

In any case, I suppose the damage is done and losing some
coverage for now on my Debian stable systems is fine...

>  t/www_listing.t | 34 ++++++++++++++++++++--------------
>  1 file changed, 20 insertions(+), 14 deletions(-)
> 
> diff --git a/t/www_listing.t b/t/www_listing.t
> index bf35530f3494c016..6a2892de9b827fe6 100644
> --- a/t/www_listing.t
> +++ b/t/www_listing.t
> @@ -122,22 +122,27 @@ SKIP: {
>  
>  	my $grok_pull = which('grok-pull') or
>  		skip('grok-pull not available', 12);
> +	my ($grok_version) = (xqx([$grok_pull, "--version"])
> +			=~ /(\d+)\.(?:\d+)(?:\.(\d+))?/);
> +	$grok_version >= 2 or
> +		skip('grok-pull v2 or later not available', 12);
>  
>  	ok(mkdir("$tmpdir/mirror"), 'prepare grok mirror dest');
>  	open $fh, '>', "$tmpdir/repos.conf" or die;
>  	print $fh <<"" or die;
> -# You can pull from multiple grok mirrors, just create
> -# a separate section for each mirror. The name can be anything.
> -[test]
> -site = http://$host:$port
> -manifest = http://$host:$port/manifest.js.gz
> +[core]
>  toplevel = $tmpdir/mirror
> -mymanifest = $tmpdir/local-manifest.js.gz
> +manifest = $tmpdir/local-manifest.js.gz
> +[remote]
> +site = http://$host:$port
> +manifest = \${site}/manifest.js.gz
> +[pull]
> +[fsck]
>  
>  	close $fh or die;
>  
>  	xsys($grok_pull, '-c', "$tmpdir/repos.conf");
> -	is($? >> 8, 127, 'grok-pull exit code as expected');
> +	is($? >> 8, 0, 'grok-pull exit code as expected');

In particular, I'm relying on this exit code in at least one of
my scripts.  Now I'll have to RTFM to figure out if I should be
testing any other exit codes or something else...

Anyways, applied and pushed as
44138460e53f90426476fa0c323fc15ef17568df for now;
but not happy this was needed.

Thanks.

Re: [PATCH 3/3] t/www_listing: require grok-pull version 2 or later

[Konstantin Ryabitsev]

On Sun, Feb 21, 2021 at 10:20:13PM +0000, Eric Wong wrote:
> > This was tested with the latest release of Grokmirror, v2.0.7.  Note
> > that the "pull" and "fsck" sections are required even though they're
> > empty.

Hmm... That grok-pull requires the [fsck] section is a bug that I introduced
in one of the last versions. :/ I'll put in a fix that doesn't require it.

> >   Another option would be to generate an appropriate v1 or v2
> >   configuration based on which Grokmirror version is detected.  I'm
> >   not sure that's worth the trouble though.
> 
> Ugh, some of these incompatible changes to grokmirror are really
> annoying and will break existing scripts when I upgrade.
> (and I suspect this affects other people, too).

Grokmirror-1.x is not maintained any more and 2.x is a much better codebase
anyway.

It was a difficult call to introduce a config change that intentionally
breaks upgrade path from 1.x to 2.x, but the priority was to avoid a much
bigger problem by sneaking this upgrade on an unsuspecting repo admin.
Grokmirror-2 completely refactors how backend repositories are organized, so
the first grok-fsck run on large repo collections with a lot of alternates
could have had potentially led to a disastrous outcome resulting in repository
corruption. Not a problem for repos on lore.kernel.org where there are no
forks, but very much a problem for git.kernel.org and source.codeaurora.org
mirrors (especially the latter).

So, I had to make the choice between annoyance and potential corruption. :)
The breaking nature of the change is documented in
https://git.sr.ht/~monsieuricon/grokmirror/tree/master/item/UPGRADING.rst

> >  	xsys($grok_pull, '-c', "$tmpdir/repos.conf");
> > -	is($? >> 8, 127, 'grok-pull exit code as expected');
> > +	is($? >> 8, 0, 'grok-pull exit code as expected');
> 
> In particular, I'm relying on this exit code in at least one of
> my scripts.  Now I'll have to RTFM to figure out if I should be
> testing any other exit codes or something else...

I'm pretty sure I did this to make grok-pull work better with systemd, sorry.

-K