Publish hotrod image to docker hub #702

pavolloffay · 2018-02-20T13:27:02Z

Resolves #204

Signed-off-by: Pavol Loffay ploffay@redhat.com

yurishkuro · 2018-02-20T14:09:08Z

.travis.yml

@@ -50,6 +53,7 @@ script:
  - if [ "$CROSSDOCK" == true ]; then bash ./scripts/travis/build-crossdock.sh ; else echo 'skipping crossdock'; fi
  - if [ "$DOCKER" == true ]; then bash ./scripts/travis/build-docker-images.sh ; else echo 'skipping docker images'; fi
  - if [ "$ES_INTEGRATION_TEST" == true ]; then bash ./scripts/travis/es-integration-test.sh ; else echo 'skipping elastic search integration test'; fi
+  - if [ "$HOTROD" == true ]; then bash ./scripts/travis/es-integration-test.sh ; else echo 'skipping hotrod example'; fi


this is calling es-integration-test

pavolloffay · 2018-02-20T16:27:23Z

Makefile

+
+.PHONE: docker-hotrod
+docker-hotrod: build-examples
+	docker build -t $(DOCKER_NAMESPACE)/example-hotrod:${DOCKER_TAG} ./examples/hotrod


example-hotrod

hotrod-example

hotrod

I created example-hotrod repo on Hub

pavolloffay · 2018-02-20T16:27:43Z

examples/hotrod/Dockerfile

@@ -1,4 +1,4 @@
 FROM scratch
 EXPOSE 8080 8081 8082 8083
-COPY hotrod-linux /
-CMD ["./hotrod-linux", "all"]
+COPY hotrod-linux /go/bin/


changing this to align with other images

pavolloffay · 2018-02-20T16:29:03Z

examples/hotrod/README.md

 go get github.com/jaegertracing/jaeger
 cd $GOPATH/src/github.com/jaegertracing/jaeger
 make install
 cd examples/hotrod
 go run ./main.go all
 ```

+### Run HotROD from docker
+```bash
+docker run --rm -it --link jaeger -p8080:8080 jaegertracing/example-hotrod:latest /go/bin/hotrod-linux all --jaeger-agent.host-port=jaeger:6831


if we use entrypoint then this would look like docker run --rm -it --link jaeger -p8080:8080 jaegertracing/example-hotrod:latest --jaeger-agent.host-port=jaeger:6831

yurishkuro · 2018-02-20T19:10:49Z

unit tests failed twice in the same spot

go test -coverprofile .cover/cover.72.out -v -race github.com/jaegertracing/jaeger/plugin/storage
=== RUN   TestFactoryConfigFromEnv
--- PASS: TestFactoryConfigFromEnv (0.00s)
=== RUN   TestFactoryConfigFromEnvDeprecated
--- PASS: TestFactoryConfigFromEnvDeprecated (0.00s)
=== RUN   TestNewFactory
--- FAIL: TestNewFactory (0.00s)
	<autogenerated>:1: 
			Error Trace:	factory_test.go:59
			Error:      	Should NOT be empty, but was &{<nil> <nil> <nil>}
			Test:       	TestNewFactory

pavolloffay · 2018-02-21T10:56:18Z

This failure is not related to this PR, I get it on master locally.

EDIT: there has been a change in testify and NotEmpty fails if struct contains only nil references.

coveralls · 2018-02-21T11:30:06Z

Coverage remained the same at 100.0% when pulling 83b7c23 on pavolloffay:publish-hotrod into ff666cd on jaegertracing:master.

yurishkuro · 2018-02-21T16:15:02Z

examples/hotrod/README.md

 go get github.com/jaegertracing/jaeger
 cd $GOPATH/src/github.com/jaegertracing/jaeger
 make install
 cd examples/hotrod
 go run ./main.go all
 ```

+### Run HotROD from docker
+```bash
+docker run --rm -it --link jaeger -p8080-8083:8080-8083 jaegertracing/example-hotrod:latest /go/bin/hotrod-linux all --jaeger-agent.host-port=jaeger:6831


can we use syntax with ENV var? I think it will make the command less intimidating.

yes, I think there are two variants:

use viper to configure this

define env var in dockerfile and pass it to this flag (defaults to localhost) - only for agent host port as it will be used the most

I don't think we need viper in hotrod it will complicate code. I will add env var

yurishkuro · 2018-02-21T16:16:49Z

examples/hotrod/services/frontend/server.go

@@ -60,7 +60,12 @@ func (s *Server) createServeMux() http.Handler {

 func (s *Server) home(w http.ResponseWriter, r *http.Request) {
 	s.logger.For(r.Context()).Info("HTTP", zap.String("method", r.Method), zap.Stringer("url", r.URL))
-	http.ServeFile(w, r, "services/frontend/web_assets/index.html")
+	b, err := assetFS().Asset("web_assets/index.html")


shouldn't this be loaded once in the constructor?

yurishkuro · 2018-02-21T16:18:12Z

scripts/travis/hotrod-integration-test.sh

+export CID=$(docker run -d -p 8080:8080 $REPO:latest)
+body=$(curl localhost:8080)
+if [[ $body != *"Rides On Demand"* ]]; then
+  exit 1


should log something to explain why it's failing

yurishkuro · 2018-02-21T16:20:28Z

EDIT: there has been a change in testify and NotEmpty fails if struct contains only nil references.

We should upgrade testify (and also use a semver instead of head) in a separate PR and fix the test, so that this PR is clean.

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2018-03-15T12:37:26Z

@yurishkuro regarding #702 (comment)

can we use syntax with ENV var? I think it will make the command less intimidating.

We have two options: use viper which will complicate code or use env var in dockerfile like:

FROM alpine
ENV JAEGER_AGENT_HOST_PORT localhost:6831
EXPOSE 8080 8081 8082 8083
COPY hotrod-linux /go/bin/
CMD /go/bin/hotrod-linux all --jaeger-agent.host-port ${JAEGER_AGENT_HOST_PORT}

It will use shell cmd command, instead of exec. Or other solution is to use

FROM scratch
EXPOSE 8080 8081 8082 8083
COPY hotrod-linux /go/bin/
ENTRYPOINT ["/go/bin/hotrod-linux", "all"]

and it will be used as docker run jaegertracing/hotrod --jaeger-agent.host-port host:port

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro · 2018-03-15T12:44:35Z

Makefile

-build-examples:
-	go build -o ./examples/hotrod/hotrod-demo ./examples/hotrod/main.go
+build-examples: install-go-bindata
+	cd ./examples/hotrod/services/frontend/ && go-bindata-assetfs -pkg frontend web_assets/... && cd -


not sure if it works in make, but using parens (cd x && cmd) automatically scopes the execution making cd - unnecessary

pavolloffay · 2018-03-15T13:11:54Z

@yurishkuro what about my comment regarding env #702 (comment).

I like more the second approach with entrypoint. I think nobody will use hotrod with other than all command

yurishkuro · 2018-03-15T13:22:22Z

yes I like the second option better

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay · 2018-03-16T12:13:56Z

All passing ready for the final review/merge

yurishkuro

just one question before we merge - what's the reason for having a separate matrix entry for hotrod (and all-in-one, for that matter) instead of uploading all docker images from the DOCKER=true entry? Each entry makes the build longer since we have limited number of workers and travis initialization takes a few minutes.

pavolloffay · 2018-03-21T16:23:30Z

To test it if it works. We could refactor it in #707.

I will merge it now. We can always remove it.

pavelnikolov · 2018-03-22T13:36:48Z

Thank you for fixing this! I'll update the Kubernetes Chart to use the official hotrod Docker image.

pavolloffay requested review from black-adder, vprithvi and yurishkuro as code owners February 20, 2018 13:27

ghost assigned pavolloffay Feb 20, 2018

ghost added the review label Feb 20, 2018

yurishkuro reviewed Feb 20, 2018

View reviewed changes

pavolloffay force-pushed the publish-hotrod branch from d8bd653 to fc09869 Compare February 20, 2018 16:25

pavolloffay commented Feb 20, 2018

View reviewed changes

pavolloffay force-pushed the publish-hotrod branch from 83fae78 to ae73f9c Compare February 21, 2018 11:40

yurishkuro reviewed Feb 21, 2018

View reviewed changes

pavolloffay mentioned this pull request Feb 22, 2018

Pin testify version to 1.2.1 #710

Merged

pavolloffay force-pushed the publish-hotrod branch from ae73f9c to 4037319 Compare February 23, 2018 16:47

ghost assigned yurishkuro Mar 1, 2018

pavolloffay force-pushed the publish-hotrod branch from fced2f8 to 8438bfb Compare March 15, 2018 12:25

pavolloffay added 2 commits March 15, 2018 13:28

Publish hotrod to dockerub

5ab659a

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix review comments

70effb0

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the publish-hotrod branch from 8438bfb to e22ba4f Compare March 15, 2018 12:28

Add env to dockerfile to handle collector hostport

59c7ee0

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

pavolloffay force-pushed the publish-hotrod branch from e22ba4f to 59c7ee0 Compare March 15, 2018 12:29

Use alpine

4cebe9d

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro reviewed Mar 15, 2018

View reviewed changes

pavolloffay added 6 commits March 15, 2018 14:37

Use scratch and entrypoint hotrod all

9bf9c3f

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Remove cd -

9b35a5a

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Fix cd

3946ea4

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add ls

8cef0bb

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Remove bindata.go

1df257d

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

Add wait until 200

83b7c23

Signed-off-by: Pavol Loffay <ploffay@redhat.com>

yurishkuro approved these changes Mar 16, 2018

View reviewed changes

pavolloffay merged commit 30d3e18 into jaegertracing:master Mar 21, 2018

ghost removed the review label Mar 21, 2018

eundoosong mentioned this pull request Mar 22, 2018

Package static assets inside the query-service binary #609

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Publish hotrod image to docker hub #702

Publish hotrod image to docker hub #702

pavolloffay commented Feb 20, 2018

yurishkuro Feb 20, 2018

pavolloffay Feb 20, 2018

yurishkuro Feb 21, 2018

pavolloffay Feb 20, 2018

pavolloffay Feb 20, 2018

yurishkuro commented Feb 20, 2018

pavolloffay commented Feb 21, 2018 •

edited

Loading

coveralls commented Feb 21, 2018 •

edited

Loading

yurishkuro Feb 21, 2018

pavolloffay Feb 23, 2018

pavolloffay Feb 23, 2018

yurishkuro Feb 21, 2018

yurishkuro Feb 21, 2018

yurishkuro commented Feb 21, 2018

pavolloffay commented Mar 15, 2018

yurishkuro Mar 15, 2018

pavolloffay commented Mar 15, 2018

yurishkuro commented Mar 15, 2018

pavolloffay commented Mar 16, 2018

yurishkuro left a comment

pavolloffay commented Mar 21, 2018

pavelnikolov commented Mar 22, 2018

Publish hotrod image to docker hub #702

Publish hotrod image to docker hub #702

Conversation

pavolloffay commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Feb 20, 2018

pavolloffay commented Feb 21, 2018 • edited Loading

coveralls commented Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro commented Feb 21, 2018

pavolloffay commented Mar 15, 2018

Choose a reason for hiding this comment

pavolloffay commented Mar 15, 2018

yurishkuro commented Mar 15, 2018

pavolloffay commented Mar 16, 2018

yurishkuro left a comment

Choose a reason for hiding this comment

pavolloffay commented Mar 21, 2018

pavelnikolov commented Mar 22, 2018

pavolloffay commented Feb 21, 2018 •

edited

Loading

coveralls commented Feb 21, 2018 •

edited

Loading