Build and test images #65

ravimeda · 2018-01-04T14:29:17Z

Unit tests to validate the framework images are needed. Such validation is available in dotnet-docker. This PR lays the foundation to build and test the framework images. Current changes use ImageBuilder to build the framework images locally and in CI. A subsequent PR will bring the unit tests.

Fixes #66

ravimeda · 2018-01-04T14:33:51Z

build-and-test.ps1

+Set-StrictMode -Version Latest
+$ErrorActionPreference = 'Stop'
+
+$(docker version) | ForEach-Object { Write-Host "$_" }


Docker version is emitted here and in ImageBuilder. Either move this statement into the if block where ImageBuilder image is pulled (or) remove version emit in ImageBuilder itself. Any preferences? #Closed

Since ImageBuilder is emitting, I don't think this is necessary at all. #Resolved

Agree. My thinking earlier was - Docker is a prerequisite for the build-test experience. If a prerequisite is not available, in this case Docker version command fails, then exit immediately. #Closed

ravimeda · 2018-01-04T14:40:24Z

build-and-test.ps1

+}
+if (-not [string]::IsNullOrEmpty($OSFilter))
+{
+    $buildFilter = "$buildFilter$OSFilter/*"


-OSFilter 1709 would match all 1709 Dockerfiles. So one might expect -OSFilter ltsc would match all ltsc. But it wouldn't. This is due to 2016 suffix, which means -OSFilter ltsc2016 is how the parameter value should be for LTSC images.
Any preferences on the usage? #Closed

Seems like there should be a - if an OSFilter is specified but not a VersionFilter - e.g. $buildFilter-$OSFilter/*.

I think it is just fine that -OSFilter ltsc is not supported. The user can specify *ltsc* for this to work. #Resolved

*ltsc* sounds fine.

Hyphen - is not required. But would make it more accurate. I will update this as follows -

$buildFilter = "*" if (-not [string]::IsNullOrEmpty($VersionFilter)) { $buildFilter = "$VersionFilter-$buildFilter" } if (-not [string]::IsNullOrEmpty($OSFilter)) { if ([string]::IsNullOrEmpty($VersionFilter)) { $buildFilter = "$buildFilter-$OSFilter/*" } else { $buildFilter = "$buildFilter$OSFilter/*" } } ``` #Closed

ravimeda · 2018-01-04T14:42:07Z

netci.groovy

+def branch = GithubBranchName
+def isPR = true
+def platformList = ['Windows_2016:WindowsServerCore-ltsc2016', 'Windows_2016:WindowsServerCore-1709']
+def versionList = ['3.5', '4.6.2', '4.7.1']


Each time a new version is added/removed, one has to edit this list. In a way it is good to revisit the coverage but there is maintenance cost. #Closed

Shouldn't 4.7 be included? #Resolved

This is the same pattern used in .NET Core. It has to be this way if we want to run each version as a separate leg. I think the real issue here is the CI system in that it is incapable of picking up changes to CI immediately. #Resolved

Just realized there is an issue here - 3.5 and 4.7.1 are the only versions supported on 1709. You can see it in https://github.com/Microsoft/dotnet-framework-docker/blob/master/build-pipeline/pipeline.json. CI is going to have to account for this. #Resolved

Good point! I will update accordingly. #Closed

MichaelSimons · 2018-01-04T15:19:39Z

netci.groovy

+def branch = GithubBranchName
+def isPR = true
+def platformList = ['Windows_2016:WindowsServerCore-ltsc2016', 'Windows_2016:WindowsServerCore-1709']
+def versionList = ['3.5', '4.6.2', '4.7.1']


Shouldn't 4.7 be included? #Resolved

MichaelSimons · 2018-01-04T15:21:37Z

netci.groovy

+def branch = GithubBranchName
+def isPR = true
+def platformList = ['Windows_2016:WindowsServerCore-ltsc2016', 'Windows_2016:WindowsServerCore-1709']
+def versionList = ['3.5', '4.6.2', '4.7.1']


This is the same pattern used in .NET Core. It has to be this way if we want to run each version as a separate leg. I think the real issue here is the CI system in that it is incapable of picking up changes to CI immediately. #Resolved

MichaelSimons · 2018-01-04T15:23:03Z

netci.groovy

+    def machineLabel = 'latest-docker'
+
+    versionList.each { version ->
+        def newJobName = Utilities.getFullJobName(project, "${containerOS}_${version}", isPR)


I think it may be easier to reason over this name if it were changed to ${version}-${containerOS} because this will align with the folder names #Resolved

Ok. will update. #Closed

MichaelSimons · 2018-01-04T15:25:09Z

netci.groovy

@@ -0,0 +1,32 @@
+import jobs.generation.Utilities


You are going to need to make a PR to change https://github.com/dotnet/dotnet-ci/blob/master/data/repolist.txt as well in order to get CI enabled. #Resolved

Done. #Closed

MichaelSimons · 2018-01-04T15:27:19Z

build-and-test.ps1

+}
+
+$imageBuilderExe = [System.IO.Path]::Combine($imageBuilderFolder, "image-builder", "Microsoft.DotNet.ImageBuilder.exe")
+$imageBuilderArgs = 'build --repo microsoft/dotnet-framework --path "$buildFilter"'


This doesn't look correct - --repo microsoft/dotnet-framework. This causes microsoft/dotnet-framework-build to be skipped. I don't think the --repo filter should be specified at all. #Resolved

You are correct. repo is not needed for build command. It is needed for other commands such as the one that generates and updates readme. #Closed

MichaelSimons · 2018-01-04T15:28:18Z

build-and-test.ps1

+Set-StrictMode -Version Latest
+$ErrorActionPreference = 'Stop'
+
+$(docker version) | ForEach-Object { Write-Host "$_" }


Since ImageBuilder is emitting, I don't think this is necessary at all. #Resolved

MichaelSimons · 2018-01-04T15:29:41Z

build-and-test.ps1

+    $buildFilter = "$buildFilter$OSFilter/*"
+}
+
+$imageBuilderFolder = ".Microsoft.DotNet.ImageBuilder"


I think it may be better to explicitly add the PSScriptRoot to this path so that there is consistence in case the script is not run from root.
#Resolved

Ok. Will update. #Closed

MichaelSimons · 2018-01-04T15:31:28Z

build-and-test.ps1

+}
+
+$imageBuilderFolder = ".Microsoft.DotNet.ImageBuilder"
+if (-not (Test-Path -Path "$imageBuilderFolder" -PathType Container))


I think we should consider checking for the presence of the exe. If any of the docker steps fail, the script should retry on additional runs.
#Resolved

Good point! Will update accordingly. #Closed

MichaelSimons · 2018-01-04T16:15:50Z

build-and-test.ps1

+}
+if (-not [string]::IsNullOrEmpty($OSFilter))
+{
+    $buildFilter = "$buildFilter$OSFilter/*"


Seems like there should be a - if an OSFilter is specified but not a VersionFilter - e.g. $buildFilter-$OSFilter/*.

I think it is just fine that -OSFilter ltsc is not supported. The user can specify *ltsc* for this to work. #Resolved

Address PR feedback - 1

ravimeda · 2018-01-05T02:20:00Z

build-and-test.ps1

+        try {
+            New-Item -Path "$imageBuilderFolder" -ItemType Directory -Force | Out-Null
+
+            Invoke-Expression "docker pull $imageBuilderImageName"


As discussed offline, it would be simpler if ImageBuilder binaries were available as a compressed file hosted in Azure storage. Acquiring ImageBuilder would then be simpler - download and extract,

Logged dotnet/docker-tools#67

MichaelSimons · 2018-01-05T18:40:50Z

netci.groovy

+    def(hostOS, containerOS) = platform.tokenize(':')
+    def machineLabel = 'latest-docker'
+
+    if (containerOS == 'WindowsServerCore-1709') {


Per 1f61282, this can now be updated to a single static list - ['3.5', '4.*'] #Resolved

Ok. Updated as def versionList = ['3.5', '4.*']

MichaelSimons · 2018-01-05T18:42:57Z

build-and-test.ps1

+}
+
+$imageBuilderFolder = [System.IO.Path]::Combine("$PSScriptRoot", ".Microsoft.DotNet.ImageBuilder")
+$imageBuilderExe = [System.IO.Path]::Combine($imageBuilderFolder, "image-builder", "Microsoft.DotNet.ImageBuilder.exe")


Why create the image-builder sub folder? This seems to add unnecessary nesting. #Resolved

As discussed offline, this is due to moby/moby#34638

MichaelSimons · 2018-01-05T18:45:53Z

build-and-test.ps1

+        try {
+            New-Item -Path "$imageBuilderFolder" -ItemType Directory -Force | Out-Null
+
+            Invoke-Expression "docker pull $imageBuilderImageName"


I think the docker pull is the only thing that should be in the retry. It is the only call that is intermittent. If the other cmds fail, something is drastically wrong and a retry isn't going to succeed.. #Resolved

Ok. Will move rest of the command outside wait-retry block.

MichaelSimons · 2018-01-05T18:49:00Z

build-and-test.ps1

+    Get-ImageBuilder
+}
+
+$imageBuilderArgs = 'build --path "$buildFilter"'


Now that the tests have been checked in, you will need to pass the OSFilter and VersionFilter as build-args. #Resolved

Clarification - did you mean invoke tests as in -
./tests/run-test.ps1 -VersionFilter $VersionFilter -OSFilter $OSFilter

I mean you will need to pass in `--test-var VersionFilter=$VersionFilter --test-var OSFilter=$OSFilter' into ImageBuilder #Resolved

Ok. I didn't notice the recent commit (e618fa1) that introduced testCommands section in the manifest. Yes, I will pass those args.

MichaelSimons · 2018-01-05T18:51:48Z

build-and-test.ps1

+$ErrorActionPreference = 'Stop'
+
+$buildFilter = "*"
+if (-not [string]::IsNullOrEmpty($VersionFilter))


What do you think about defaulting VersionFilter and OSFilter to '*' if not specified? Then this logic could be simplified to - buildFilter = "{VersionFilter}-{OSFilter}/*" #Resolved

Yes, this works. Thanks!

MichaelSimons · 2018-01-05T23:18:49Z

build-and-test.ps1

+    $imageBuilderContainerName = "ImageBuilder-$(Get-Date -Format yyyyMMddhhmmss)"
+    New-Item -Path "$imageBuilderFolder" -ItemType Directory -Force | Out-Null
+
+    $attempt = 0


I think it would help the readability to factor out the logic to perform the retry into a separate function. #Resolved

MichaelSimons · 2018-01-08T15:14:58Z

build-and-test.ps1

-    $imageBuilderImageName = 'microsoft/dotnet-buildtools-prereqs:image-builder-nanoserver-20180105103709'
-    $imageBuilderContainerName = "ImageBuilder-$(Get-Date -Format yyyyMMddhhmmss)"
-    New-Item -Path "$imageBuilderFolder" -ItemType Directory -Force | Out-Null
+function ExecuteCommand {


I think it would be helpful if the name called out that that retry logic exists. This helps clarify why the function is used over Invoke-Expression. e.g. ExecuteWithRetry #Resolved

I did consider the name ExecuteWithRetry. However, with the word Retry the questions that come to my (user's) mind is - how do I set the number of retries, wait time? Is the wait time in seconds or minutes, etc.

Since this is a local method, I think it is reasonable for the reader to look at the implementation. I think ExecuteWithRetry addresses the point I raised on why the method exists over using the built in Invoke-Expression and why this method is only being used for the pull cmd.

MichaelSimons · 2018-01-08T15:17:31Z

build-and-test.ps1

-if ($IsDryRun)
-{
+$imageBuilderArgs = "build --path $buildFilter --test-var VersionFilter=$VersionFilter --test-var OSFilter=$OSFilter"
+if ($IsDryRun) {


What do you think about replacing the IsDryRun parameter with a string CustomImageBuilderArgs? This would make this script more robust so that it could be used within the official builds. The CustomImageBuilderArgs could be used to specify the push related arguments. #Resolved

To clarify - change the type of the parameter to string, and append the parameter value to the default args. So in that case, I think ImageBuilderCustomArgs would convey better, and the logic would be -

$imageBuilderArgs = "build --path $buildFilter --test-var VersionFilter=$VersionFilter --test-var OSFilter=$OSFilter" if ([string]::IsNullOrWhiteSpace($ImageBuilderCustomArgs)) { $imageBuilderArgs += " $ImageBuilderCustomArgs" }

yes that is what I was thinking - and I like your suggested name.

ravimeda · 2018-01-08T18:54:07Z

@dotnet-bot test ci please

…-docker into utests

ravimeda · 2018-01-08T20:07:53Z

@dotnet-bot test ci please

ravimeda · 2018-01-08T22:52:22Z

Thanks @mmitche for enabling CI!

Build and test images

fdc0468

ravimeda self-assigned this Jan 4, 2018

ravimeda requested a review from MichaelSimons January 4, 2018 14:29

ravimeda commented Jan 4, 2018

View reviewed changes

MichaelSimons requested changes Jan 4, 2018

View reviewed changes

ravimeda added 2 commits January 4, 2018 20:00

Address PR feedback - 1

d9000b5

Merge pull request #1 from ravimeda/utests-pr

82b5aab

Address PR feedback - 1

ravimeda commented Jan 5, 2018

View reviewed changes

MichaelSimons reviewed Jan 5, 2018

View reviewed changes

MichaelSimons requested changes Jan 5, 2018

View reviewed changes

ravimeda added 2 commits January 5, 2018 15:53

Merge branch 'master' into utests

ab19e49

Address PR feedback - 2 (#2)

b596eee

MichaelSimons reviewed Jan 5, 2018

View reviewed changes

Address PR feedback - 3

c4511e9

MichaelSimons reviewed Jan 8, 2018

View reviewed changes

Address PR feedback - 4

33d195b

MichaelSimons approved these changes Jan 8, 2018

View reviewed changes

ravimeda added 2 commits January 8, 2018 14:00

Remove astrix from versionList def

7575d9c

Merge branch 'utests' of https://github.com/ravimeda/dotnet-framework…

48389c8

…-docker into utests

ravimeda merged commit dfe728b into microsoft:master Jan 8, 2018

ravimeda deleted the utests branch February 13, 2018 16:07

Build and test images #65

Build and test images #65

Conversation

ravimeda commented Jan 4, 2018 • edited by MichaelSimons

ravimeda Jan 4, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

ravimeda Jan 4, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

ravimeda Jan 4, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

ravimeda Jan 5, 2018 • edited

Choose a reason for hiding this comment

MichaelSimons Jan 4, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons Jan 5, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons Jan 5, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons Jan 5, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons Jan 5, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons Jan 5, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons Jan 5, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MichaelSimons Jan 5, 2018 • edited by ravimeda

Choose a reason for hiding this comment

MichaelSimons Jan 8, 2018 • edited by ravimeda

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravimeda commented Jan 4, 2018 •

edited by MichaelSimons

ravimeda Jan 4, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

ravimeda Jan 4, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

ravimeda Jan 4, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

MichaelSimons Jan 4, 2018 •

edited by ravimeda

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

MichaelSimons Jan 4, 2018 •

edited by ravimeda

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

ravimeda Jan 5, 2018 •

edited

MichaelSimons Jan 4, 2018 •

edited by ravimeda

MichaelSimons Jan 5, 2018 •

edited by ravimeda

MichaelSimons Jan 5, 2018 •

edited by ravimeda

MichaelSimons Jan 5, 2018 •

edited by ravimeda

MichaelSimons Jan 5, 2018 •

edited by ravimeda

MichaelSimons Jan 5, 2018 •

edited by ravimeda

MichaelSimons Jan 5, 2018 •

edited by ravimeda

MichaelSimons Jan 5, 2018 •

edited by ravimeda

MichaelSimons Jan 8, 2018 •

edited by ravimeda

MichaelSimons Jan 8, 2018 •

edited by ravimeda