-
Notifications
You must be signed in to change notification settings - Fork 148
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
Use binary complied by make for e2e tests. #130
Use binary complied by make for e2e tests. #130
Conversation
hack/e2e.sh should use the "official" binary created by make instead of its own build 1. we should test the real binary 2. "make test; make; make container" (as our travis does) should use the real binary instead of the wrong one built by "make test"
@@ -38,7 +38,7 @@ container: $(APP) | |||
push: container | |||
docker push $(IMAGE_NAME):$(IMAGE_VERSION) | |||
|
|||
test: | |||
test: $(APP) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message says "make test; make; make container", but that won't work anymore, right? It's perhaps better to put "make; make test; make container" into the commit message and mention in the Makefile for "test" that one has to do "make" first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the order does not really matter. All three make
, make test
and make container
depend on $(APP)
. So the first one executed compiles the binary, the others re-use it. You may notice I removed make
in this PR, because it's useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it works here the way you said.
I was expecting "$(APP)" to be a phony target because there is no real dependency tracking for Go+make, i.e. "make $(APP)" always has to invoke "go build" to ensure that the binary really is up-to-date. But this Makefile here only builds the binary if it doesn't exist yet, without rebuilding it when it is out-dated.
I guess that's how it is meant to be here, so /lgtm.
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Use binary complied by make for e2e tests.
hack/e2e.sh
should use the "official" binary created by make instead of its own buildmake test; make; make container
(as our travis does) should use the real binary instead of the wrong one built bymake test
@pohly @lpabon, PTAL
Fixes: #127