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

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).

docker-tools/src/Microsoft.DotNet.ImageBuilder/src/DockerService.cs

Lines 59 to 60 in f2d23ef

    
           public Task<IEnumerable<string>> GetImageManifestLayersAsync(string image, IRegistryCredentialsHost credsHost, bool isDryRun) => 
        
               _manifestService.GetImageLayersAsync(image, credsHost, isDryRun);

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).

docker-tools/src/Microsoft.DotNet.ImageBuilder/src/DockerServiceCache.cs

Lines 55 to 56 in f2d23ef

    
           public Task<string?> GetImageDigestAsync(string image, IRegistryCredentialsHost credsHost, bool isDryRun) => 
        
               _imageDigestCache.GetImageDigestAsync(image, credsHost, isDryRun);

docker-tools/src/Microsoft.DotNet.ImageBuilder/src/ImageDigestCache.cs

Lines 37 to 43 in f2d23ef

    
           public Task<string?> GetImageDigestAsync(string tag, IRegistryCredentialsHost credsHost, bool isDryRun) => 
        
               LockHelper.DoubleCheckedLockLookupAsync(_digestCacheLock, _digestCache, tag, 
        
                   () => _dockerService.GetImageDigestAsync(tag, credsHost, isDryRun), 
        
                   // Don't allow null digests to be cached. A locally built image won't have a digest until 
        
                   // it is pushed so if its digest is retrieved before pushing, we don't want that  
        
                   // null to be cached. 
        
                   val => !string.IsNullOrEmpty(val));

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/

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	public Task<IEnumerable<string>> GetImageManifestLayersAsync(string image, IRegistryCredentialsHost credsHost, bool isDryRun) =>
	_manifestService.GetImageLayersAsync(image, credsHost, isDryRun);

	public Task<string?> GetImageDigestAsync(string image, IRegistryCredentialsHost credsHost, bool isDryRun) =>
	_imageDigestCache.GetImageDigestAsync(image, credsHost, isDryRun);

	public Task<string?> GetImageDigestAsync(string tag, IRegistryCredentialsHost credsHost, bool isDryRun) =>
	LockHelper.DoubleCheckedLockLookupAsync(_digestCacheLock, _digestCache, tag,
	() => _dockerService.GetImageDigestAsync(tag, credsHost, isDryRun),
	// Don't allow null digests to be cached. A locally built image won't have a digest until
	// it is pushed so if its digest is retrieved before pushing, we don't want that
	// null to be cached.
	val => !string.IsNullOrEmpty(val));

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions