-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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 snapshot tests for internal Dockerfiles #6142
base: nightly
Are you sure you want to change the base?
Conversation
@@ -10,7 +10,7 @@ | |||
set isFullAzureLinux to isAzureLinux && !isDistroless ^ | |||
set isDistrolessAzureLinux to isAzureLinux && isDistroless ^ | |||
set baseUrl to VARIABLES[cat("dotnet|", dotnetVersion, "|base-url|", VARIABLES["branch"])] ^ | |||
set isInternal to find(baseUrl, "artifacts.visualstudio.com") >= 0 ^ | |||
set isInternal to VARIABLES["IsInternal"] || find(baseUrl, "artifacts.visualstudio.com") >= 0 ^ |
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.
If possible, I'd like to avoid having to depend on an IsInternal
variable to keep things simple. Is it possible for the Get-GeneratedDockerfiles
script to override the URL instead so that it matches with artifacts.visualstudio.com
? Or for the test to update the content of the manifest.versions.json file?
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.
That was the first thing that I tried. I ran into lots and lots of issues getting the right combination of escape characters to pass arguments through with the correct --var variable="foo"
syntax. It ended up wasting lots of time. I added the new override variable just to get things working. I can give this one more try though.
[GeneratedRegex(@"alpine\d+\.\d+")] | ||
private static partial Regex AlpineVersionRegex { get; } | ||
|
||
private static List<(Regex pattern, string replacement)> Patterns { get; } = |
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.
private static List<(Regex pattern, string replacement)> Patterns { get; } = | |
private static List<(Regex pattern, string replacement)> PatternReplacements { get; } = |
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.
Can you add some comments to this file explaining what it's all about? It's not clear to me.
Not sure why there is a diff with the generated internal dockerfiles here. I can't reproduce the test failure locally. The CI output should ideally show the text diff as well, which would make this easier to diagnose. I think the issue there is that the library doesn't think that Dockerfiles are text files (because they have no file extension), so it doesn't try to show the diff. I'll look into it more. |
Fixes #5935
Uses https://github.com/VerifyTests/Verify to implement snapshot testing for internal Dockerfiles.
Internal Dockerfiles are scrubbed of versions and shas, and then checked-in to
tests/Microsoft.DotNet.Docker.Tests/Baselines/
. When tests are ran, internal Dockerfiles will be generated and compared to the checked-in versions. If there is a diff, you'll have the opportunity to review and accept differences.The easiest way to accept or reject new changes is using Verify.Terminal in the repo root:
Todo: