Skip to content
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

Manifest publishing support for duplicated platforms #666

Merged
merged 2 commits into from
Oct 2, 2020

Conversation

mthalman
Copy link
Member

@mthalman mthalman commented Oct 1, 2020

With the changes being done for dotnet/dotnet-docker#2182, it will cause a platform to be defined multiple times in a manifest, contained in separate images. This requires changes to PublishManfiestCommand in order to correctly handle this scenario because it will otherwise throw an exception when it encounters a platform with no concrete platform tags.

This is solved by finding the matching platform that does contain a concrete tag and using that as the reference tag for the manifest creation.

@mthalman mthalman requested a review from MichaelSimons October 1, 2020 19:15
@mthalman mthalman marked this pull request as draft October 1, 2020 19:38
@mthalman
Copy link
Member Author

mthalman commented Oct 1, 2020

Moving this to draft because I found some issues that need fixing.

@mthalman
Copy link
Member Author

mthalman commented Oct 1, 2020

I've also fixed an issue I found with the merging of image info content. Duplicated platforms contained in separate images were being detected as equal in PlatformData.Equals. This meant there content would be merged together instead of kept in separate images.

The solution is not great. It now takes into account the convention a duplicated platform will not have simple tags defined for it. When one platform has simple tags and another does not, they are now considered to not be equal regardless of whether they having a matching Dockerfile, platform, etc.

@mthalman mthalman marked this pull request as ready for review October 1, 2020 21:22
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing this issue triggered me to question if the IngestKustoImageInfoCommand is going to handle this situation. I think it should but can you take a look if you haven't already to ensure the correct info is getting ingested?

@mthalman
Copy link
Member Author

mthalman commented Oct 2, 2020

Seeing this issue triggered me to question if the IngestKustoImageInfoCommand is going to handle this situation. I think it should but can you take a look if you haven't already to ensure the correct info is getting ingested?

It would result in duplicate digest lines outputted. Is that ok?

@MichaelSimons
Copy link
Member

Seeing this issue triggered me to question if the IngestKustoImageInfoCommand is going to handle this situation. I think it should but can you take a look if you haven't already to ensure the correct info is getting ingested?

It would result in duplicate digest lines outputted. Is that ok?

Yes, duplicated digests will be alright.

@mthalman mthalman merged commit f31211a into dotnet:master Oct 2, 2020
@mthalman mthalman deleted the dd-issue2182 branch October 2, 2020 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants