-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update image info schema to have image/platform concepts #437
Conversation
The proposed schema makes me think there is an existing issue with the current schema that is not addressed with the new schema. That is using the Dockerfile path as the key to the image/platform Dictionary is an issue. Specially in the case of the samples where we build the same Dockerfile multiple time - https://github.com/dotnet/versions/blob/master/build-info/docker/image-info.dotnet-dotnet-docker-master.json#L1477. To address this, should the image collection be an array and the Dockerfile be an attribute of the platform? |
#286 tracks the Dockerfile path issue I previously mentioned. I think we should be addressing this problem with this schema change. |
Yes, I've made that change. This required adding some more data to the The schema will now look like this:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, I have just a couple minor comments.
This is a type of change I think we should consider rolling out to nightly to validate before pushing it to the other repos. Perhaps it would make sense to create a feature branch to put these ImageInfo changes into and test with nightly then push out to the other repos. Creating a feature branch would allow us to make servicing fixes in master concurrently that can get pushed out to all of the repos.
src/Microsoft.DotNet.ImageBuilder/src/Commands/CopyAcrImagesCommand.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.ImageBuilder/src/Models/Image/PlatformData.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.DotNet.ImageBuilder/src/Models/Image/ImageData.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - Any thoughts on my previous suggestion to use a feature branch?
Oh right, thanks for the reminder. I'll do that. |
In order to support data like shared tags in the image info file, the schema needs to be updated to group Dockerfiles associated with a single manifest tag just as the manifest does. This grouping is referred to as an image while each of the underlying items are referred to as a platform. I've updated the schema to reflect this by renaming the previously named
ImageData
toPlatformData
because that is really what it represents now and introducing a newImageData
class that groups thePlatformData
instances.OLD schema example:
NEW schema example:
The main impact this change causes is with the merging behavior. There needs to be a way to determine which image instance a platform belongs when merging a source to a target. The image has no identifying data. This is accomplished by correlating in memory each image in an image info file with its related image in the manifest file. That can then be used as a reference identifier to pair up related images between two separate image info files.
Fixes #286