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

Adds newline after unit-tests target. #10837

Merged
merged 1 commit into from Apr 6, 2020
Merged

Adds newline after unit-tests target. #10837

merged 1 commit into from Apr 6, 2020

Conversation

Weil0ng
Copy link
Contributor

@Weil0ng Weil0ng commented Apr 2, 2020

It seems that in some env if the path is large enough for the full list
of files, the full bash command in that target gets too big for bash and
hence will trigger an error too many arguments. (figured out by
@aanm and @joestringer )

Fixes: #10836

Signed-off-by: Weilong Cui cuiwl@google.com

@Weil0ng Weil0ng requested a review from a team as a code owner April 2, 2020 19:37
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 2, 2020

@joestringer @aanm

@joestringer joestringer added pending-review release-note/misc This PR makes changes that have no direct user impact. labels Apr 2, 2020
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Couple of minor nits.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@joestringer
Copy link
Member

@Weil0ng for this issue since it fixes #10836, if you write "Fixes: #10836" in the PR or commit message then github will autoclose the other issue when this is merged.

@coveralls
Copy link

coveralls commented Apr 2, 2020

Coverage Status

Coverage decreased (-0.01%) to 45.888% when pulling 3b51a86 on Weil0ng:newline into e774b9c on cilium:master.

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 2, 2020

PTAL

@joestringer
Copy link
Member

test-me-please

@pchaigno
Copy link
Member

pchaigno commented Apr 3, 2020

I think the Travis CI failure is related:

go test -mod=vendor -ldflags "-X github.com/cilium/cilium/pkg/kvstore.consulDummyConfigFile=/tmp/cilium-consul-certs/cilium-consul.yaml -X github.com/cilium/cilium/pkg/testutils.CiliumRootDir=/home/travis/gopath/src/github.com/cilium/cilium -X github.com/cilium/cilium/pkg/datapath.DatapathSHA=1234567890abcdef7890 -X github.com/cilium/cilium/pkg/logging.DefaultLogLevelStr="error"" github.com/cilium/cilium/bugtool -test.v -timeout 360s -coverprofile=coverage.out -covermode=count -coverpkg ./... || exit 1; tail -n +2 coverage.out >> coverage-all-tmp.out;
/bin/bash: 2: unbound variable
Makefile:152: recipe for target 'unit-tests' failed
make: *** [unit-tests] Error 1
The command "./.travis/build.sh" exited with 2.

It was also failing with the same error on the previous build, for aarch64 only as well.

@aanm
Copy link
Member

aanm commented Apr 3, 2020

@Weil0ng it seems is a legit failure in travis:

go test -mod=vendor -ldflags "-X github.com/cilium/cilium/pkg/kvstore.consulDummyConfigFile=/tmp/cilium-consul-certs/cilium-consul.yaml -X github.com/cilium/cilium/pkg/testutils.CiliumRootDir=/home/travis/gopath/src/github.com/cilium/cilium -X github.com/cilium/cilium/pkg/datapath.DatapathSHA=1234567890abcdef7890 -X github.com/cilium/cilium/pkg/logging.DefaultLogLevelStr="error"" github.com/cilium/cilium/bugtool -test.v -timeout 360s -coverprofile=coverage.out -covermode=count -coverpkg ./... || exit 1; tail -n +2 coverage.out >> coverage-all-tmp.out;
/bin/bash: 2: unbound variable
Makefile:152: recipe for target 'unit-tests' failed
make: *** [unit-tests] Error 1

@Weil0ng Weil0ng force-pushed the newline branch 2 times, most recently from 46f44fb to dbfb96a Compare April 3, 2020 19:59
@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 3, 2020

PTAL. Have to use another workaround to cope with different versions of bash.

@joestringer
Copy link
Member

Travis still seems unhappy, so I'll defer review until it is green.

It seems that in some env if the path is large enough for the full list
of files, the full bash command in that target gets too big for bash and
hence will trigger an error ``too many arguments``.

Signed-off-by: Weilong Cui <cuiwl@google.com>
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM

@joestringer
Copy link
Member

test-me-please

@Weil0ng
Copy link
Contributor Author

Weil0ng commented Apr 4, 2020

hmm, that's odd, the failure seems like unrelated to this PR...

@joestringer
Copy link
Member

@Weil0ng It may be. Usually we will try to look for issues with the test name and reference the issue so that we can track common flakes on master and make progress towards resolving them.

@pchaigno
Copy link
Member

pchaigno commented Apr 6, 2020

Previous run hit #10821.

test-with-kernel

@tgraf tgraf merged commit cfa665d into cilium:master Apr 6, 2020
1.8.0 automation moved this from In progress to Merged Apr 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

Unable to run make unit-tests locally
6 participants