-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Publish hotrod image to docker hub #702
Publish hotrod image to docker hub #702
Conversation
.travis.yml
Outdated
@@ -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 |
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.
this is calling es-integration-test
d8bd653
to
fc09869
Compare
|
||
.PHONE: docker-hotrod | ||
docker-hotrod: build-examples | ||
docker build -t $(DOCKER_NAMESPACE)/example-hotrod:${DOCKER_TAG} ./examples/hotrod |
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.
example-hotrod
hotrod-example
hotrod
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 created example-hotrod
repo on Hub
@@ -1,4 +1,4 @@ | |||
FROM scratch | |||
EXPOSE 8080 8081 8082 8083 | |||
COPY hotrod-linux / | |||
CMD ["./hotrod-linux", "all"] | |||
COPY hotrod-linux /go/bin/ |
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.
changing this to align with other images
examples/hotrod/README.md
Outdated
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 |
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.
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
unit tests failed twice in the same spot
|
This failure is not related to this PR, I get it on master locally. EDIT: there has been a change in |
83fae78
to
ae73f9c
Compare
examples/hotrod/README.md
Outdated
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 |
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.
can we use syntax with ENV var? I think it will make the command less intimidating.
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.
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
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 don't think we need viper in hotrod it will complicate code. I will add env var
@@ -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") |
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.
shouldn't this be loaded once in the constructor?
export CID=$(docker run -d -p 8080:8080 $REPO:latest) | ||
body=$(curl localhost:8080) | ||
if [[ $body != *"Rides On Demand"* ]]; then | ||
exit 1 |
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.
should log something to explain why it's failing
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. |
ae73f9c
to
4037319
Compare
fced2f8
to
8438bfb
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
8438bfb
to
e22ba4f
Compare
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
e22ba4f
to
59c7ee0
Compare
@yurishkuro regarding #702 (comment)
We have two options: use viper which will complicate code or use env var in dockerfile like:
It will use shell cmd command, instead of exec. Or other solution is to use
and it will be used as |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Makefile
Outdated
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 - |
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.
not sure if it works in make, but using parens (cd x && cmd)
automatically scopes the execution making cd -
unnecessary
@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 |
yes I like the second option better |
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
Signed-off-by: Pavol Loffay <ploffay@redhat.com>
All passing ready for the final review/merge |
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.
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.
To test it if it works. We could refactor it in #707. I will merge it now. We can always remove it. |
Thank you for fixing this! I'll update the Kubernetes Chart to use the official hotrod Docker image. |
Resolves #204
Signed-off-by: Pavol Loffay ploffay@redhat.com