search

dotnet / docker-tools

This is a repo to house some common tools for our various docker repos.
MIT License
103 stars 42 forks source link

DockerService, ManifestService, ImageDigestCache, and RegistryServiceClient classes are all too tightly coupled #1265

Open lbussell opened 2 months ago

lbussell commented 2 months ago

At a high level, DockerService depends on the ManifestService. However, DockerService only relies on the ManifestService for methods that have the exact same signature and behavior as ManifestService commands (or its extension methods).

https://github.com/dotnet/docker-tools/blob/f2d23ef5d1312903ae8de3a9cc095b7e74422f8b/src/Microsoft.DotNet.ImageBuilder/src/DockerService.cs#L59-L60

The DockerServiceCache is also tightly coupled with the ImageDigestCache class because of the same reason. ImageDigestCache simply makes and caches calls to the ManifestService class (through DockerService).

https://github.com/dotnet/docker-tools/blob/f2d23ef5d1312903ae8de3a9cc095b7e74422f8b/src/Microsoft.DotNet.ImageBuilder/src/DockerServiceCache.cs#L55-L56

https://github.com/dotnet/docker-tools/blob/f2d23ef5d1312903ae8de3a9cc095b7e74422f8b/src/Microsoft.DotNet.ImageBuilder/src/ImageDigestCache.cs#L37-L43

We should remove DockerService's dependency on the ManifestService class, since there are commands that use the DockerService that don't rely on any functionality from the ManifestService. ImageDigestCache should become ManifestServiceCache (or potentially RegistryServiceClientCache, since that is the only class making the single network call). We should experiment with combining ManifestService and RegistryServiceClient.

Also, ManifestServiceExtensions can be combined into the ManifestService class since they are in the same namespace.

I have put together a branch where I experimented with removing the DockerService dependency on the ManifestService, but it is currently incomplete and not fully functional - https://github.com/lbussell/docker-tools/commits/dev/loganbussell/1262playground/

dotnet-issue-labeler[bot] commented 2 months ago

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

lbussell commented 2 months ago

[Triage] We may want to consider keeping the ManifestServiceExtensions class, since its existence allows us to keep the IManifestService interface as small as possible which reduces the complexity of testing the ManifestService. The methods in the ManifestServiceExtensions don't rely on any of the ManifestService's internal state.