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

Use digest for sqlserver and fix tests #5059

Closed
wants to merge 2 commits into from

Conversation

joperezr
Copy link
Member

@joperezr joperezr commented Jul 24, 2024

An update to mssql/server image tag 2022-latest broke our tests as we can no longer communicate with the container. Chatting offline with our experts from the dotnet-docker repo, they suggested we should instead depend on digests as opposed to tags, to be able to guarantee determinism, and then just make sure we update these digests in a frequent basis to ensure we get fixes for them. This PR is switching just the sqlserver hosting component to use digest instead and it also does that for the tests so that they pass again, but we will likely want to make an overhaul pass to all of our hosting components to similarly depend on digests as opposed to tags.

cc: @mitchdenny @radical @eerhardt

Microsoft Reviewers: Open in CodeFlow

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-integrations Issues pertaining to Aspire Integrations packages label Jul 24, 2024
@@ -27,7 +27,8 @@ public static IResourceBuilder<SqlServerServerResource> AddSqlServer(this IDistr
var sqlServer = new SqlServerServerResource(name, passwordParameter);
return builder.AddResource(sqlServer)
.WithEndpoint(port: port, targetPort: 1433, name: SqlServerServerResource.PrimaryEndpointName)
.WithImage(SqlServerContainerImageTags.Image, SqlServerContainerImageTags.Tag)
.WithImage(SqlServerContainerImageTags.Image)
.WithImageSHA256(SqlServerContainerImageTags.Digest)
Copy link
Member

@eerhardt eerhardt Jul 24, 2024

Choose a reason for hiding this comment

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

they suggested we should instead depend on digests as opposed to tags, to be able to guarantee determinism

Note that this is OK to unblock our tests, but this doesn't comply with our long term plan.

#3933
#1997 (comment)

This approach doesn't allow for security patches to be automatically applied.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's have a follow up discussion just to finalize on our plan going forward. We may very well end up following the same plan we have, I just want to make sure we take into consideration the concept of determinism and the fact that we have now been broken by this, so we should check what our customers would expect us to do.

For this particular case, it's worth noting that the underlying image doesn't follow semver which seems to be one of the keys of our plan.

@joperezr
Copy link
Member Author

Closing in favor of #5058 where @radical was able to keep the tests enabled without the need to use digests. We should still figure out how to fix this not only for our tests, but also for our hosting component.

@joperezr joperezr closed this Jul 25, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Aug 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-integrations Issues pertaining to Aspire Integrations packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants