Libguestfs - May 2022 - S3 plugin test case breaks test suite / development workflow...

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.

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.

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.

I think we should improve our process here.

Thanks,
Laszlo

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

On May 17 2022, Laszlo Ersek <lersek at redhat.com>
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.
Apologies, I missed that. I think Richard has already fixed this since.


Best,
-Nikolaus

-- 
GPG Fingerprint: ED31 791B 2C5C 1613 AF38 8B8A D113 FCAC 3C4E 599F

             ?Time flies like an arrow, fruit flies like a Banana.?