Introduce search_content method for repositories [DELIVERY-7839]

[negillett]

This commit adds the search_content method for the Repository class
which searches for and retrieves content units from Pulp repositories.

[negillett]

Not sure why it's complaining about coverage. The tests included do cover these lines.

[rohanpm]

I think you're missing support on the fake client - if you have a Repository obtained from a fake client now and you call search_content, what happens? That should work too & would need a test.

[midnightercz]

I wonder if we should merge this and #64 together. Most of the work would be done for arranging code to support model criteria anyway. And it seems to be useless to implement something which will be overwritten in next PR

[negillett]

I wonder if we should merge this and #64 together.

I think I'd rather see #64 replace the criteria implemented here with unit models rather than include all the work necessary to make that work in this issue.

[negillett]

I think you're missing support on the fake client - if you have a Repository obtained from a fake client now and you call search_content, what happens? That should work too & would need a test.

The repository object is still real, right? And, though it makes sense that the real client isn't being called, at least search_content should be covered as it belongs to Repository.
I'm not understanding how it isn't being reported as covered or how client._search was previously covered if only the fake client is used in tests.
Also, apparently all the Unit subclasses were covered but I see no tests for them. So, I'm not sure how to get ModulemdDefaultUnit covered either.

[rohanpm]

I think you're missing support on the fake client - if you have a Repository obtained from a fake client now and you call search_content, what happens? That should work too & would need a test.

The repository object is still real, right? And, though it makes sense that the real client isn't being called, at least search_content should be covered as it belongs to Repository.

The point is that if you use a fake client to get a Repository object, then call search_content, it must behave in a way which makes sense, i.e. it returns a consistent view of whatever data is stored on the fake client. Does it behave that way now? I'd expect it's necessary to write new code to achieve that, and then that new code will need to be tested.

I'm not understanding how it isn't being reported as covered or how client._search was previously covered if only the fake client is used in tests.

It's not the case that only the fake client is used in tests.

This library provides two implementations of a Pulp client for use by others, one designed to issue real HTTP requests to a Pulp server and the other designed to return data from local memory. Both implementations should be tested by this library's test suite, so there are some tests here using one and some tests using the other.

Also, apparently all the Unit subclasses were covered but I see no tests for them. So, I'm not sure how to get ModulemdDefaultUnit covered either.

Autotest tests/model/test_model_invariants.py automatically finds and performs some basic tests on every model class.

On top of that, at least one test where a search returns a ModulemdDefaultUnit would be nice, which I think you already have added. I don't think there's much need for adding more tests when introducing new unit types.

[rohanpm]

It doesn't seem to work at all against a real Pulp server.

(python3) [rmcgover@picallow pubtools-pulplib]$ python3
Python 3.7.4 (default, Jul  9 2019, 16:32:37) 
[GCC 9.1.1 20190503 (Red Hat 9.1.1-1)] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from pubtools.pulplib import Client
>>> c = Client('https://rhsm-pulp-dev/', auth=('admin', 'admin'), verify=False)
>>> repo = c.get_repository('all-iso-content')

# Library won't accept Pulp server's type IDs
>>> repo.search_content(type_id='iso')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/rmcgover/src/pubtools-pulplib/pubtools/pulplib/_impl/model/repository/base.py", line 223, in search_content
    raise InvalidContentTypeException()
pubtools.pulplib._impl.model.common.InvalidContentTypeException

# And Pulp server won't accept library's type IDs
>>> repo.search_content(type_id='FileUnit')
<ProxyFuture at 0x7f9078f69750 state=pending>
...
Retrying due to error: 400 Client Error: Bad Request for url: https://rhsm-pulp-dev/pulp/api/v2/repositories/all-iso-content/search/units/ [1/10]
Traceback (most recent call last):
  File "/home/rmcgover/src/more-executors/more_executors/_impl/map.py", line 61, in _delegate_resolved
    result = self._map_fn(result)
  File "/home/rmcgover/src/pubtools-pulplib/pubtools/pulplib/_impl/client/client.py", line 412, in _unpack_response
    pulp_response.raise_for_status()
  File "/home/rmcgover/dev/python3/lib/python3.7/site-packages/requests/models.py", line 940, in raise_for_status
    raise HTTPError(http_error_msg, response=self)
requests.exceptions.HTTPError: 400 Client Error: Bad Request for url: https://rhsm-pulp-dev/pulp/api/v2/repositories/all-iso-content/search/units/

Can you please ensure it works against a real server?

To make this easier for yourself and reviewers, you can consider adding a simple script under examples, as was done for some other additions to API.

[rohanpm]

The main problem that I'm having is that coveralls is claiming that nothing I've implemented is covered, and that just doesn't seem possible based on the tests I've included and what you've said here.

I can reproduce the coveralls results locally.

• modulemd_defaults.py isn't tested because it's never imported. It also means that it's not covered by the test_model_invariants.py test I mentioned - a class has to at least be present in the library's public API for it to be found & tested. It also won't appear in the documentation. You can handle it similarly to ModulemdUnit to fix these problems.
• Although you do have a test which will load data of type modulemd_defaults, you never verify that the resulting object is an instance of ModulemdDefaultsUnit, so I think the library is falling into the default case it takes for all units of unknown type, which is to return an instance of the Unit base class. If you add at least one test which can only pass if the returned unit is genuinely a ModulemdDefaultsUnit, it would protect against this bug.
• The test class you added, TestSearchContent, isn't being run at all. I can see this from the output of py.test (need -v option to list each test function run). If you want to make a test class, you apparently need to subclass unittest.TestCase, which hasn't been done here. But I would rather recommend following the same style as existing tests in this repo, using bare functions named test_* and not unnecessarily wrapped into a class.

[rohanpm]

I can reproduce the coveralls results locally.
(... 3 points)

Points 2 and 3 are probably conflicting with each other a bit, as I was reviewing the TestSearchContent body with the understanding that it would be executed, which later turned out to be false. If you fix point 3 first, it might fix or partially fix point 2.

[negillett]

[...] coveralls results [...]

Yeah, I figured out a lot of this myself last night and think I've solved most of the points you mention.

Fixed

[negillett]

Finally.
Please review, @release-engineering/content-delivery.

[JayZ12138]

I gave +1 for this patch, but for the repo.search_content API, I still think this is too much. It calls the client.search_repo_content api, means they are completely the same, so why make two of them? Rohan's API design says, 'when in doubt, leave it out', since it's easy to add but difficult to remove.

Anyway, I'm OK if you insist, I gave +1 :)

[negillett]

@jbruzl, I think you're the only code owner for this repo still around.

[jbruzl]

Approving as all code owners are not available.