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

integ-cli: fix TestCommitChange for pulled busybox #11006

Merged
merged 1 commit into from
Mar 2, 2015
Merged

integ-cli: fix TestCommitChange for pulled busybox #11006

merged 1 commit into from
Mar 2, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Feb 25, 2015

If the tests are running outside a container (i.e.
executed without make test), we are using a busybox
pulled from Docker Hub (instead of jpetazzo's docker-busybox).

The one pulled from Docker Hub always adds an extra
PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
env var all and that messes up the test TestCommitChange.
That's currently breaking the windows CI. (The test is introduced in #9123.)

I'm keeping the same PATH here but making it explicit
so that it's always set and we verify what we set. It's actually the same
thing if I set ENV PATH foo here but I thought it may lead to some
problems hard to debug in the future.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
cc: @tiborvass @dqminh @tianon @cpuguy83

@dqminh
Copy link
Contributor

dqminh commented Feb 25, 2015

I think its' better to use inspectFieldJSON and comparing two maps ( like in docker_cli_build_test.go ) instead of just comparing string here as that is more stable and we would not have this problem. ( order-independent )

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 25, 2015

@dqminh well there's gonna be an extra environment variable in the output. Is that an expected outcome in the test (i.e. part of the test)? If not, we can do what you said.

@dqminh
Copy link
Contributor

dqminh commented Feb 25, 2015

@ahmetalpbalkan the mismatch is caused by different busybox images. The official busybox image has PATH setup in its env, i guess the reason is because it was committed from a live container which will have a default PATH if not set https://github.com/docker/docker/blob/master/daemon/container.go#L1286. But i dont know about the official image build process so i can definitely be wrong here.

The patch itself ( overriding PATH ) looks good and i think we can do it. I just think that it's more stable if we unmarshal the Env json and compare two maps directly.

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 25, 2015

@dqminh if we use inspectFieldJSON, the test code starts filling up with in-place hacks like json decoding, reflect.DeepEquals and weird type comparisons full of interface{}. The code turns from 10-line code (which scales with number of fields inspected) into a less-scalable and unreadable like this one –and it still doesn't provide any features like order of the array doesn't matter etc:

    {
        // Validate Config.ExposedPorts
        res, err := inspectFieldJSON(imageId, "Config.ExposedPorts")
        if err != nil {
            t.Fatal(err)
        }

        var exposedPorts map[string]interface{}
        if err := json.Unmarshal([]byte(res), &exposedPorts); err != nil {
            t.Fatalf("failed to unmarshal value: %v", err)
        }

        if expected := map[string]interface{}{"8080/tcp": map[string]interface{}{}}; !reflect.DeepEqual(exposedPorts, expected) {
            t.Fatalf("Expected: %#v\nGot:%#v", expected, exposedPorts)
        }
    }
    {
        // Validate Config.Env
        res, err := inspectFieldJSON(imageId, "Config.Env")
        if err != nil {
            t.Fatal(err)
        }

        var env []string
        if err := json.Unmarshal([]byte(res), &env); err != nil {
            t.Fatalf("failed to unmarshal value: %v", err)
        }

        if expected := []string{"DEBUG=true", "test=1", "PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"}; !reflect.DeepEqual(env, expected) {
            t.Fatalf("Expected: %#v\nGot:%#v", expected, env)
        }
    }

Let me know which one seems better to you? 😄 My personal preference is towards string comparison as we do currently...

@dqminh
Copy link
Contributor

dqminh commented Feb 27, 2015

@ahmetalpbalkan right, if we use inspectField we would have to do the marshal dance ! I was just paranoid. On a second look, i think Env is saved as an array with the same order we specified in --change list, so the current code should not have order issue.

So the current approach looks good.

@@ -253,6 +253,7 @@ func TestCommitChange(t *testing.T) {
"--change", "EXPOSE 8080",
"--change", "ENV DEBUG true",
"--change", "ENV test 1",
"--change", "ENV PATH /usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin",
Copy link
Contributor

Choose a reason for hiding this comment

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

can we use a different PATH here to be sure that it overrides the default PATH ? ( i.e., just /usr/sbin:/usr/bin:/sbin:/bin )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually planning to use /foo, just kept it like this to keep things working and making things easy to debug if something goes wrong (I don't know how that can happen..). How does /foo sound? It will make it obvious we don't actually care about the value.

Copy link
Contributor

Choose a reason for hiding this comment

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

/foo is good too. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

If the tests are running outside a container (i.e.
executed without `make test`), we are using a `busybox`
pulled from Docker Hub (not jpatezzo's docker-busybox).

That one adds an extra
`PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin`
env var all the time and that messes the test `TestCommitChange`.
That's currently breaking the windows CI.

I'm keeping the same PATH here but making it explicit
so that it's always set and we verify what we set. It's actually the same
thing if I set `ENV PATH foo` here but I thought it may lead to some
problems hard to debug in the future.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@dqminh
Copy link
Contributor

dqminh commented Feb 27, 2015

LGTM ( but im not a maintainer 😄 ). It's interesting to know the exact reason why hub's busybox is different from the image built with docker build too (i.e. extra PATH)

@duglin
Copy link
Contributor

duglin commented Feb 27, 2015

That a little misleading to have the busybox used in the testcases be different from what people see when they use it outside of our test env - not to mention perhaps even hiding bugs.
Just curious, did you try to make it so that we used the same one?

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 27, 2015

Just curious, did you try to make it so that we used the same one?

not sure what you mean? can you please elaborate @duglin?

@duglin
Copy link
Contributor

duglin commented Feb 27, 2015

if I'm understanding this correctly, this error is happening because of the difference between the busybox on the hub vs the busybox that's used in the testing, right?

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 27, 2015

@duglin yes. In #10799, we decided to pull busybox from Hub if we are not tests inside container.

@duglin
Copy link
Contributor

duglin commented Feb 27, 2015

So, it seems like the real issue here might be that our testing from inside the container is using a different busybox, right? And if so, have we tried to make it so that we're using the same one (from the hub) in both cases?

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 27, 2015

@duglin that's a valid question. @tianon any ideas why we still import docker-busybox repo rather than pulling the same thing from docker hub in the Dockerfile? (please don't tell me it's because github uptime is higher than docker hub. 😄)

@LK4D4
Copy link
Contributor

LK4D4 commented Feb 27, 2015

@ahmetalpbalkan But you have no docker in Dockerfile to pull image.

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 27, 2015

@LK4D4 ah, right. I forgot the fact that we load docker-busybox at test-time after binary build. https://github.com/docker/docker/blob/2768ce0e4e773d0a0331c6294f31fd9ff9450062/project/make/.ensure-busybox#L6 feeling stupid lol. Anyway this fixes the problem comes from there and I'm not really concerned that much about the difference of these two images.

@tianon
Copy link
Member

tianon commented Feb 27, 2015

LGTM

@tianon
Copy link
Member

tianon commented Feb 27, 2015

Uhh:

=== RUN TestCommitChange
[PASSED]: commit - commit --change
--- FAIL: TestCommitChange (2.25s)
    docker_cli_commit_test.go:276: Config.ExposedPorts('<no value>'), expected map[8080/tcp:map[]]
    docker_cli_commit_test.go:276: Config.Env('[PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin]'), expected [DEBUG=true test=1 PATH=/foo]

(from that windows jank run)

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 27, 2015

@tianon I hate this. Will look into it sometime. Don't merge it yet folks.

@tianon
Copy link
Member

tianon commented Feb 27, 2015

😢

@tianon
Copy link
Member

tianon commented Feb 28, 2015

I think that failure is a Windows-specific bug -- I've tested on Linux with the image from the Hub, and it has this same issue, but this fix works properly there.

@tianon
Copy link
Member

tianon commented Feb 28, 2015

(in other words, IMO this should merge as-is since it's a good, valid fix and whatever is causing Windows to fail should be a new PR)

@jessfraz
Copy link
Contributor

jessfraz commented Mar 2, 2015

LGTM

jessfraz pushed a commit that referenced this pull request Mar 2, 2015
…e-fix

integ-cli: fix TestCommitChange for pulled busybox
@jessfraz jessfraz merged commit bedf3f8 into moby:master Mar 2, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants