Use existing Windows image for test instead of microsoft/nanoserver by StefanScherer · Pull Request #39095 · moby/moby

StefanScherer · 2019-04-17T07:52:28Z

- What I did

Fix the test TestPullWindowsImageFailsOnLinux as the latest tag is no longer available as seen in https://jenkins.dockerproject.org/job/Docker-PRs/53794/console
Also improve the tests to check no matching manifest for ... does not contain unknown.

- How I did it

- How to verify it

make test-integration

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

the animal is invisible as the latest tag

integration-cli/docker_cli_pull_test.go

codecov · 2019-04-17T10:20:44Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1742b9d). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master   #39095   +/-   ##
=========================================
  Coverage          ?   36.99%           
=========================================
  Files             ?      612           
  Lines             ?    45390           
  Branches          ?        0           
=========================================
  Hits              ?    16794           
  Misses            ?    26317           
  Partials          ?     2279

StefanScherer · 2019-04-17T10:21:54Z

Updated the test to the ltsc2019 servercore image which should exist for a long time.

thaJeztah · 2019-04-17T11:00:55Z

integration-cli/docker_cli_pull_test.go

Shouldn't this produce the other error though?

moby/distribution/config.go

Lines 154 to 160 in fc01c2b

// fail immediately on Windows when downloading a non-Windows image

// and vice versa. Exception on Windows if Linux Containers are enabled.

if runtime.GOOS == "windows" && unmarshalledConfig.OS == "linux" && !system.LCOWSupported() {

return nil, fmt.Errorf("image operating system %q cannot be used on this platform", unmarshalledConfig.OS)

} else if runtime.GOOS != "windows" && unmarshalledConfig.OS == "windows" {

return nil, fmt.Errorf("image operating system %q cannot be used on this platform", unmarshalledConfig.OS)

}

It actually shows this error when the image in the registry is a manifest list.

moby/distribution/pull_v2.go

Line 759 in 50e63ad

errMsg := fmt.Sprintf("no matching manifest for %s in the manifest list entries", formatPlatform(platform))

Hm... looks like a bug in that detection then; wondering: could we fix that?
Was the previous image not multi-arch?

Actually; for this test we're only checking that the pull fails, so I guess it's ok (we might want to check for no matching manifest for linux perhaps?)

Excellent idea so we don't need a new test for #38574 :-)
I'll add it, but also the no matching manifest for windows in the corresponding test for Windows.

Signed-off-by: Stefan Scherer <[email protected]>

thaJeztah

LGTM

andrewhsu · 2019-04-17T15:55:06Z

integration-cli/docker_cli_pull_test.go

 	testRequires(c, DaemonIsLinux, Network)
-	_, _, err := dockerCmdWithError("pull", "microsoft/nanoserver")
-	assert.ErrorContains(c, err, "cannot be used on this platform")
+	_, _, err := dockerCmdWithError("pull", "mcr.microsoft.com/windows/servercore:ltsc2019")


When I try this with docker-ce 19.03.0-beta1 on a linux machine:

$ docker pull mcr.microsoft.com/windows/servercore:ltsc2019 ltsc2019: Pulling from windows/servercore no matching manifest for linux/amd64 in the manifest list entries

However, when I try this on docker for mac with docker-ce 18.09.3 engine:

$ docker pull mcr.microsoft.com/windows/servercore:ltsc2019 ltsc2019: Pulling from windows/servercore no matching manifest for unknown in the manifest list entries

I think this PR is good to go if expecting the arch in the error message is the right thing to expect.

Seeing the same on Docker for Mac, so looks like there might be an issue there (or perhaps something we fixed since?)

PR #38574 that fixes that unknown message doesn't have a 18.09 tag, so it comes with 19.03

Ah, right! I knew there was one; let me have a look at backporting it to 18.09

Backport is up; docker-archive#197

cpuguy83

LGTM... though it may be nice to have a fake non-matching image in a local registry...

GordonTheTurtle added the status/0-triage label Apr 17, 2019

thaJeztah reviewed Apr 17, 2019

View reviewed changes

integration-cli/docker_cli_pull_test.go Outdated Show resolved Hide resolved

StefanScherer force-pushed the fix-TestPullWindowsImageFailsOnLinux branch from 778168f to a147d83 Compare April 17, 2019 10:20

StefanScherer changed the title ~~Use microsoft/nanoserver:sac2016 instead of latest~~ Use existing Windows image for test instead of microsoft/nanoserver Apr 17, 2019

thaJeztah reviewed Apr 17, 2019

View reviewed changes

thaJeztah added area/testing platform/windows status/2-code-review and removed status/0-triage labels Apr 17, 2019

Use current windows servercore image

aad7e97

Signed-off-by: Stefan Scherer <[email protected]>

StefanScherer force-pushed the fix-TestPullWindowsImageFailsOnLinux branch from a147d83 to aad7e97 Compare April 17, 2019 13:24

StefanScherer mentioned this pull request Apr 17, 2019

Add regression tests for pull errors from a manifest list #39096

Closed

thaJeztah approved these changes Apr 17, 2019

View reviewed changes

thaJeztah added the rebuild/experimental label Apr 17, 2019

GordonTheTurtle removed the rebuild/experimental label Apr 17, 2019

andrewhsu reviewed Apr 17, 2019

View reviewed changes

cpuguy83 approved these changes Apr 17, 2019

View reviewed changes

cpuguy83 merged commit 4ed9059 into moby:master Apr 17, 2019

StefanScherer deleted the fix-TestPullWindowsImageFailsOnLinux branch April 17, 2019 17:54

thaJeztah mentioned this pull request Apr 18, 2019

[18.09 backport] test-fixes and updates, and fix images filter when use multi reference filter docker-archive/engine#198

Merged

thaJeztah added the process/cherry-picked label Jun 7, 2019

	// fail immediately on Windows when downloading a non-Windows image
	// and vice versa. Exception on Windows if Linux Containers are enabled.
	if runtime.GOOS == "windows" && unmarshalledConfig.OS == "linux" && !system.LCOWSupported() {
	return nil, fmt.Errorf("image operating system %q cannot be used on this platform", unmarshalledConfig.OS)
	} else if runtime.GOOS != "windows" && unmarshalledConfig.OS == "windows" {
	return nil, fmt.Errorf("image operating system %q cannot be used on this platform", unmarshalledConfig.OS)
	}

Conversation

StefanScherer commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

StefanScherer commented Apr 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpuguy83 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

StefanScherer commented Apr 17, 2019 •

edited

Loading

codecov bot commented Apr 17, 2019 •

edited

Loading