cncf-tags / container-device-interface

Apache License 2.0
216 stars 39 forks source link

Implement default cache with a minimal set of package-level functions. #192

Closed klihub closed 9 months ago

klihub commented 10 months ago

This patch set

klihub commented 10 months ago

@elezar @marquiz I threw together this PR based on our last community meeting discussion regarding adding top-level package functions (and an implicitly available default cache instance) for the most commonly used functionality. PTAL.

klihub commented 10 months ago

@elezar @marquiz @kad ping

klihub commented 10 months ago

Question about the default-cache package-level api: should it expose all methods of Cache as package level functions? If not, should it match the current registry API, then?

Based on our last community meeting chat my understanding was that the preference would be to only expose a minimal set of functions at this point, and this set would be limited to cache refreshing, device injection and error introspection/query. And the reasoning was that this would allow us to replace the current usage of the registry in cri-o and containerd with the package-level functions, which would be the next step we'd like to take.

klihub commented 10 months ago

Thanks @klihub.

I have left some comments. I think we can already remove some of the "extra" registry functions from the Registry interface -- or at least mark these as deprecated.

On your testing question: What are we trying to test by also testing the default functions?

Yes, that's what I'm also pondering now. If the cache implementation works then as longer as the getter works correctly and the straightforward package-level oneliner functions are not broken, then the default cache should work as well. However, I'm a pessimist-realist so I think we should get these exercised anyway, if for nothing else then for coverage %.

Should the tests for the default cache be a separate set of test cases anyway?

Yes, I think that ideally there should at least be separate top-level test cases (Test$CASE t* testing.T) for the default cache functions. But it should be fine for those to be totally minimalistic wrt. the corresponding functions testing the cache (even as simple as simply checking only for successful injection without verifying the correctness of the result which we already do separately for the Cache tests).

klihub commented 9 months ago

@elezar @marquiz I pushed an updated version with some of the test-related comments addressed. However, I'd like to add a few more tests for the default cache, so I re-marked this as a draft until I get those done.

klihub commented 9 months ago

Thanks @klihub. I have left some comments. I think we can already remove some of the "extra" registry functions from the Registry interface -- or at least mark these as deprecated. On your testing question: What are we trying to test by also testing the default functions?

Yes, that's what I'm also pondering now. If the cache implementation works then as longer as the getter works correctly and the straightforward package-level oneliner functions are not broken, then the default cache should work as well. However, I'm a pessimist-realist so I think we should get these exercised anyway, if for nothing else then for coverage %.

Should the tests for the default cache be a separate set of test cases anyway?

Yes, I think that ideally there should at least be separate top-level test cases (Test$CASE t* testing.T) for the default cache functions. But it should be fine for those to be totally minimalistic wrt. the corresponding functions testing the cache (even as simple as simply checking only for successful injection without verifying the correctness of the result which we already do separately for the Cache tests).

I created dedicated tests for the default cache.

klihub commented 9 months ago

@elezar I updated the PR according to your suggestion/our earlier discussion.

Another change I made is to remove the returned error from the package-level Configure(), since Cache.Configure() always returns nil since a few PRs ago. We can't do a backward-incompatible signature change to Cache.Configure() now, but I think in v0.7.0 we should do that.