[Libguestfs] S3 plugin test case breaks test suite / development workflow...

[Richard W.M. Jones]

On Tue, May 17, 2022 at 10:31:59AM +0200, Laszlo Ersek wrote:
> Hi,
> 
> I'm writing this about a specific problem and about a general problem.
> 
> * The specific problem is that commit 5130c43bc1f9 ("S3 plugin: add
> support for accessing multiple objects", 2022-05-12) introduced a
> dependency on the "botocore" python module, and now "make check" fails
> for me, because this module is unavailable on my system.
> 
> > Traceback (most recent call last):
> >   File "[...]/nbdkit/plugins/S3/nbdkit-S3-plugin", line 43, in <module>
> >     from botocore.exceptions import ClientError
> > ModuleNotFoundError: No module named 'botocore'
> 
> Now, while the README file says, from commit 6715c3d8b3e6 ("New plugin:
> S3 plugin for accessing disks stored in AWS S3 and Ceph.", 2020-11-14):
> 
> > For the Python plugin:
> >
> >  - python interpreter (version 3 only)
> >
> >  - python development libraries
> >
> >  - python unittest to run the test suite
> >
> >  - boto3 is required to run the S3 plugin written in Python
> 
> the "boto3" dependency had never been a "hard" one until commit
> 5130c43bc1f9. The language suggests that running the S3 plugin -- even
> for testing -- is optional.

Yeah it should definitely be optional.

> Therefore, commit 5130c43bc1f9 should have updated the test cases
> "test-S3.sh" and "test-S3-unit.sh" with a proper "requires" line (I
> assume anyway).
> 
> FWIW, the following trick in "test-S3.sh":
> 
> > # There is a fake boto3 module in test-S3/ which we use as a test
> > # harness for the plugin.
> 
> Does not work.
> 
> ... So, after all, the bug in commit 5130c43bc1f9 may have been that it
> did not add the ClientError exception type to the fake module in
> "tests/test-S3/boto3". I'm unsure; but please fix it anyway.
> 
> 
> * The generic problem is that I need to write this separate error report
> email, rather than commenting directly under the submission -- on the
> mailing list -- or the pull request -- on gitlab -- that ended up as
> commit 5130c43bc1f9. For the life of me, I just can't figure out *where*
> commit 5130c43bc1f9 was originally reviewed.

It's this one:

https://gitlab.com/nbdkit/nbdkit/-/merge_requests/9

It may be that you couldn't find it because I reviewed and merged the
requests by hand which involved rebasing them, so gitlab didn't
associate the commit with the original request.

> I think that's wrong.
> 
> ... Ultimately, I've found the patch in the MR listing at
> <https://gitlab.com/nbdkit/nbdkit/-/merge_requests?scope=all&state=all>,
> namely in MR#9 -- <https://gitlab.com/nbdkit/nbdkit/-/merge_requests/9>.
> 
> But this merge request has status *closed*, not *merged*. So even though
> a commit is given, I can't find the associated discussion because (a)
> MR#9 is not listed in the list of *merged* merge requests (only the
> "rejected" ones), (b) the commit message itself does not reference MR#9.

That's right.

> I think we should improve our process here.

I should probably have added a "Fixes" link when pushing them.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
virt-p2v converts physical machines to virtual machines.  Boot with a
live CD or over the network (PXE) and turn machines into KVM guests.
http://libguestfs.org/virt-v2v