Skip to content
This repository has been archived by the owner on Mar 11, 2020. It is now read-only.

Fixing Travis CI and use go 1.10 and later versions #11

Open
wants to merge 2 commits into
base: 17.06.x
Choose a base branch
from
Open

Fixing Travis CI and use go 1.10 and later versions #11

wants to merge 2 commits into from

Conversation

xinfengliu
Copy link

@xinfengliu xinfengliu commented Jan 6, 2020 •
edited
Loading

  • Need to upgrade go 1.10 and later to fix "go get -u github.com/golang/lint/golint" error in travis CI.
    See: go get golint failed with golang 1.8 on Travis golang/lint#421

  • Also modify integration test code to make it pass the CI when using golang 1.10.x.

  • Replace "github.com/golang/lint/golint" with "golang.org/x/lint/golint" (This is needed for golang 1.12 or later)

  • Modify Makefile to fix make test error when using go 1.12 or later.

Signed-off-by: Xinfeng Liu [email protected]

@xinfengliu xinfengliu changed the title Update travis to use go 1.10.x Update travis to use go 1.10.x for fixing CI Jan 6, 2020
@xinfengliu xinfengliu mentioned this pull request Jan 6, 2020
Open
thaJeztah
thaJeztah suggested changes Jan 6, 2020
View reviewed changes
integration-test/exec_test.go
@@ -58,7 +59,7 @@ func (cs *ContainerdSuite) TestBusyboxTopExecEcho(t *check.C) {
t.Assert(*e, checker.Equals, evt)
}

t.Assert(echop.io.stdoutBuffer.String(), checker.Equals, "Ay Caramba!")
t.Assert(strings.TrimRight(echop.io.stdoutBuffer.String(), string('\x00')), checker.Equals, "Ay Caramba!")
Copy link
Member

Choose a reason for hiding this comment

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

Was this due to a change in Go 1.10?

Copy link
Author

Choose a reason for hiding this comment

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

Yes.

.travis.yml
@@ -4,8 +4,7 @@ sudo: required
language: go

go:
- 1.8.x
- tip
- 1.10.x
Copy link
Member

Choose a reason for hiding this comment

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

I think docker 17.06 ee is currently built with Go 1.12.x, so it may be good to have that version in this list at least (but given that Go 1.12.x will reach EOL soon, probably have 1.13.x in there as well).

Any reason to remove tip?

@xinfengliu xinfengliu force-pushed the update-travis branch 2 times, most recently from 8b3ac4a to ad69426 Compare January 6, 2020 16:39
@thaJeztah
Copy link
Member

could you either squash the first two commits, or swap their order? If the second commit is needed to fix CI for changes due to updating to Go 1.10, it should either be there first commit, or go together with the actual change to update to Go 1.10 (otherwise Git bisect would be broken)

@xinfengliu xinfengliu force-pushed the update-travis branch 5 times, most recently from faee65b to b861a22 Compare January 7, 2020 05:30
@xinfengliu
Update travis to use go 1.10.x for fixing CI
d7736dd 
To fix "go get -u github.com/golang/lint/golint" error.
golang/lint#421

Also fix integratinon test when using golang 1.10.x
In AddProcessToContainer(), the returned stdout buffer has x00 padded, after it is converted to string, it still has padded zeros.
This commit trims the padded zeros in string.

Replace "github.com/golang/lint/golint" with "golang.org/x/lint/golint" from below error message:
(this is needed for golang 1.12 or later)
```
package github.com/golang/lint/golint: code in directory /home/travis/gopath/src/github.com/golang/lint/golint expects import "golang.org/x/lint/golint"
```

Signed-off-by: Xinfeng Liu <[email protected]>
@xinfengliu
Copy link
Author

@thaJeztah , thanks. I squshed the commits.
However, I could not make go 1.12 or 1.13 work, make test failed at go test -bench=. :

FAIL	github.com/containerd/containerd/runtime [build failed]
FAIL	github.com/containerd/containerd/supervisor [build failed]

Without -bench=., make test could pass. I don't know why.

@xinfengliu
Copy link
Author

The culprit is github.com/containerd/containerd/api/http/pprof. There is no test or benchmark method in that package, so it's safe to take this package out of go test.

@xinfengliu xinfengliu changed the title Update travis to use go 1.10.x for fixing CI Fixing Travis CI and use go 1.10 and later versions Jan 7, 2020
@xinfengliu
Add go 1.12, 1.13 and tip to Travis CI
6bab739 
Also fixed `make test` failure in go 1.12 and 1.13 by removing 'github.com/containerd/containerd/api/http/pprof' from 'go test -bench'.
Set parallel to 1 in `make integration-test`

Signed-off-by: Xinfeng Liu <[email protected]>
@thaJeztah
Copy link
Member

Two failures on tip. First one is interesting. (not a blocker, because it's tip, but curious what changed).

I kicked CI once more to see if it was just an intermittent failure

FAIL: start_linux_test.go:522: ContainerdSuite.TestSigkillShimReuseName
855
856start_linux_test.go:556:
857    t.Assert(*e, checker.Equals, evt)
858... obtained types.Event = types.Event{Type:"exit", Id:"top", Status:0xff, Pid:"init", Timestamp:(*timestamp.Timestamp)(0xc0002ded90)}
859... expected types.Event = types.Event{Type:"exit", Id:"top", Status:0x89, Pid:"init", Timestamp:(*timestamp.Timestamp)(0xc0002ded90)}
860
861
862----------------------------------------------------------------------
863FAIL: start_linux_test.go:569: ContainerdSuite.TestSigkillShimWhileContainerIsPaused
864
865start_linux_test.go:577:
866    t.Fatal(err)
867... Error: rpc error: code = 2 desc = oci runtime error: container with id exists: top

thaJeztah
thaJeztah approved these changes Jan 7, 2020
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

kicked CI again, but I guess we can merge after that completes (and ignore tip for now)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xinfengliu @thaJeztah