-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support for using Digest for base image #44461
base: main
Are you sure you want to change the base?
Add support for using Digest for base image #44461
Conversation
2842ab1
to
7f70916
Compare
Open question: Currently you can supply both We can leave it as is, or warn when supplying both. |
@dotnet-policy-service agree company="Cosafe Technology AB" |
I don't fully understand the CI errors but I doubt it's related to my changes. Perhaps it's a permissions error because I'm working on a fork? 🤔 🤷♂️ |
src/Containers/Microsoft.NET.Build.Containers/SourceImageReference.cs
Outdated
Show resolved
Hide resolved
This is great so far - kudos for diving in to a new codebase and doing everything almost perfectly!
I would prefer creating a warning. This warning should be raised in the
When new messages/warnings of any kind are added, we need to create them using .NET Resource strings. To do this, you create a new entry in the Strings.resx file and then trigger a build via
This is indeed just flaky CI - rerunning those legs or pushing new commits will probably clear them up. |
Thank you, but you're giving me too much credit. 😅 I basically just tried to follow the same pattern as
Would you mind choosing a
Got it, thanks! :) I'll add a test for the warning as well. Any other test you think I should add? |
The parse-related and validation-related errors are the 2xxx-series, so maybe |
It would be great to verify that a base image could be used by digest - in the EndToEnd tests, we pre-populate a few images by Tag. If we could get the Digest values for one or more of those and run an end-to-end publishing test using the Digest value of one of the pre-populated images that would be a good test. |
Any pointers to code here would be greatly appreciated! 🙏 Are these images public? Meaning, can I query/pull them and get the SHA? |
Sure - a test I would look at is Does that make sense? |
@BeyondEvil can you please add additional cases to
for:
|
@baronfel the format of including both a tag and a digest is not uncommon. This format enables to express what tag to follow while still being in control when to sync when the tag changes reference. It is used by tools like https://github.com/renovatebot/renovate.
For these reasons, I think the .NET SDK should simply ignore the tag and not issue a warning. |
@tmds thanks for bringing the details from other tools. This is enough to not do the warning, and not do the from tag/from digest work either. |
So to summarize, I should only add the missing tests? |
Maybe 😅 I'll implement what I think you're after and we can take it from there. 😊 |
7f70916
to
9f6970f
Compare
I'm guessing this is the reason why the test is failing: https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/Tasks/CreateNewImage.Interface.cs#L37 Can I just remove it? Doesn't this conflict with the above: https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs#L167 I should probably remove this comment: https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/ContainerHelpers.cs#L172 When I tried using this in my own project: <PropertyGroup>
<ContainerBaseImage>mcr.microsoft.com/dotnet/aspnet</ContainerBaseImage>
</PropertyGroup> I got the same error as the test. So the tag is required. Should I update the logic to require either tag or digest? If so, where would I put that? In the parser or CreateNewImage? Also, I'm having some trouble running these tests locally. I'm on an M3 Mac. Would appreciate some guidance on all of the above. 😅 🙏 |
If neither are specified, we could default to a tag of |
In my, perhaps not-so-humble, opinion - the It's fine if the resulting image, the output, is tagged with However for inputs, as the base image, this will lead to non-deterministic builds where one day your base image is bumped to a major version with breaking changes leading to difficult to find issues. Ask me how I know. 😅 With all that said, it seems like the command line argument defaults to |
Your argument is too use digests instead of tags for determinism. It isn't specific to |
Not sure what you mean 🤔 If I have tagged the base image with say But maybe I misunderstood your point? But you are correct. For guaranteed determinism, digests should be used. As recommended by docker: https://docs.docker.com/build/building/best-practices/#pin-base-image-versions |
Yes, this is what I meant and we have the same opinion on this. For UX of the SDK, I think good to follow the behavior of |
9f6970f
to
83e5984
Compare
Fair enough. Is this the correct placement of a default value? https://github.com/BeyondEvil/dotnet-sdk/blob/beyondevil/support-image-digest/src/Containers/Microsoft.NET.Build.Containers/Tasks/CreateNewImage.Interface.cs#L192 |
Add support for using image digest (sha256) for the base image when using the `PublishContainer` target. For example: `mcr.microsoft.com/dotnet/aspnet@sha256:6cec36412a215aad2a033cfe259890482be0a1dcb680e81fccc393b2d4069455`
83e5984
to
0a0a8b8
Compare
I'm sharing my opinion. @baronfel should make the the actual decision on the UX for the tag.
Or perhaps in |
@tmds is correct - if no Tag or Digest is specified for an image, we should default to |
Does that mean I should move the default from the interface to the parser? Is there a ”props” way of doing it or should I do it in logic/code? E.g. if empty assign it? |
@BeyondEvil after a quick look it seems I was wrong - we don't default to sdk/src/Containers/Microsoft.NET.Build.Containers/Tasks/ParseContainerProperties.cs Line 165 in 3c685aa
This will ensure that after Edit: hah, and it looks like you found the right place already. |
I think since everything that has been discussed has been adressed and tests are passing, I'm setting the PR to Ready. Should I also ping Thank you for all the help! 🙇 |
No, This is a great PR, thanks again for the initiative! I'll take a look and do PM approvals, but I'd still like some of the engineering team to take a look as well. |
I see 👍 😊
Sounds good, let me know if and when you need anything else from me. 🙌 |
src/Containers/Microsoft.NET.Build.Containers/SourceImageReference.cs
Outdated
Show resolved
Hide resolved
@BeyondEvil thanks for contributing this! This is a very useful feature! And, nice work on the PR! |
Thank you, it was my pleasure! And thank you for all the guidance! 🙇 |
@baronfel we didn't have to extend the parser in this PR to accept the digest. This means that existing SDKs allow a digest but then ignore it. We should clearly document what SDK versions are taking into account the digest and which aren't. |
Add support for using image digest (sha256) for the base image when using the
PublishContainer
target.For example:
mcr.microsoft.com/dotnet/aspnet@sha256:6cec36412a215aad2a033cfe259890482be0a1dcb680e81fccc393b2d4069455
Closes: dotnet/sdk-container-builds#448