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

Undocument .NET Monitor 9 arch-specific tags #5948

Open
wants to merge 9 commits into
base: nightly
Choose a base branch
from

Conversation

jander-msft
Copy link
Member

@jander-msft jander-msft commented Oct 3, 2024

As described in #5946, we should strive to remove the arch-specific tags for .NET Monitor 9. Due to technical limitations at this time, we must have at least one arch-specific tag for each image. Instead of removing them, these tags will be undocumented for .NET Monitor 9.

@lbussell
Copy link
Contributor

lbussell commented Oct 4, 2024

This clause in PublishImageInfoAsync is causing issues when there are no published platform-specific tags for an image: https://github.com/dotnet/docker-tools/blob/e36a91c00081ee416f4c0c1e5889172f7a53889f/src/Microsoft.DotNet.ImageBuilder/src/Commands/BuildCommand.cs#L194-L210

@mthalman
Copy link
Member

mthalman commented Oct 4, 2024

This clause in PublishImageInfoAsync is causing issues when there are no published platform-specific tags for an image: https://github.com/dotnet/docker-tools/blob/e36a91c00081ee416f4c0c1e5889172f7a53889f/src/Microsoft.DotNet.ImageBuilder/src/Commands/BuildCommand.cs#L194-L210

This actually doesn't surprise me now that I think about it. We have no test coverage in any of the commands for a scenario without concrete tags. I'm willing to bet there will be other commands that will run into issues with this. For example, this line right here will fail too: https://github.com/dotnet/docker-tools/blob/e36a91c00081ee416f4c0c1e5889172f7a53889f/src/Microsoft.DotNet.ImageBuilder/src/Commands/PublishManifestCommand.cs#L141. Before we move forward with this, we need more extensive testing in Image Builder. And the changes necessary to fix these issues are not small either.

@lbussell
Copy link
Contributor

lbussell commented Oct 4, 2024

Before we move forward with this, we need more extensive testing in Image Builder.

To me that sounds too risky for October servicing. So what should we do in the meantime? Undocument these tags and re-evaluate if removing them completely is feasible for GA?

@mthalman
Copy link
Member

mthalman commented Oct 4, 2024

Before we move forward with this, we need more extensive testing in Image Builder.

To me that sounds too risky for October servicing. So what should we do in the meantime? Undocument these tags and re-evaluate if removing them completely is feasible for GA?

Yes, it makes sense to undocument for now. And really, we only require one arch-specific tag. My opinion is to get rid of $(monitor|9.0|minor-tag)-<arch> and undocument the other one. Then we'll have to figure out next steps. I'm not optimistic of having infra support by GA.

@lbussell
Copy link
Contributor

lbussell commented Oct 4, 2024

My opinion is to get rid of $(monitor|9.0|minor-tag)-<arch> and undocument the other one. Then we'll have to figure out next steps. I'm not optimistic of having infra support by GA.

If we may not have infra support by GA, then perhaps we should undocument for 9.0 and remove for 10.0. That may be easier to explain to users than trying to explain why we only removed some tags but not all.

@mthalman
Copy link
Member

mthalman commented Oct 4, 2024

My opinion is to get rid of $(monitor|9.0|minor-tag)-<arch> and undocument the other one. Then we'll have to figure out next steps. I'm not optimistic of having infra support by GA.

If we may not have infra support by GA, then perhaps we should undocument for 9.0 and remove for 10.0. That may be easier to explain to users than trying to explain why we only removed some tags but not all.

Yeah, that's fair. So it comes down to how willing we are to invest in getting the infra to support this by GA. I would argue we've got better things to do during this period. And so simply undocumenting for 9.0 and target the removal for 10.0 would be the way to go.

@jander-msft
Copy link
Member Author

Before we move forward with this, we need more extensive testing in Image Builder.

To me that sounds too risky for October servicing. So what should we do in the meantime? Undocument these tags and re-evaluate if removing them completely is feasible for GA?

Yes, it makes sense to undocument for now. And really, we only require one arch-specific tag. My opinion is to get rid of $(monitor|9.0|minor-tag)-<arch> and undocument the other one. Then we'll have to figure out next steps. I'm not optimistic of having infra support by GA.

I'll get to work on that right now. I am also running into similar issues for the samples as well as other issues. While it was good to try to squeeze this in, I think we need some more bake time to work through the issues.

@jander-msft jander-msft changed the title Remove arch-specific tags from .NET Monitor 9 Remove/undocument arch-specific tags from .NET Monitor 9 Oct 4, 2024
manifest.json Show resolved Hide resolved
@jander-msft jander-msft changed the title Remove/undocument arch-specific tags from .NET Monitor 9 Undocument arch-specific tags from .NET Monitor 9 Oct 4, 2024
Copy link
Contributor

@lbussell lbussell left a comment

Choose a reason for hiding this comment

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

@jander-msft, the changes look good to me. Given that we are only undocumenting, should we undocument for all Monitor versions now?

And does any documentation need to be updated?

@jander-msft jander-msft changed the title Undocument arch-specific tags from .NET Monitor 9 Undocument .NET Monitor 9 arch-specific tags Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants