Skip to content
This repository was archived by the owner on May 18, 2020. It is now read-only.

Remove executable as an argument to containers#87

Merged
jpkrohling merged 4 commits intojaegertracing:masterfrom
zdicesare:feature/dockerfile-entrypoint
Jul 12, 2018
Merged

Remove executable as an argument to containers#87
jpkrohling merged 4 commits intojaegertracing:masterfrom
zdicesare:feature/dockerfile-entrypoint

Conversation

@zdicesare
Copy link
Copy Markdown
Contributor

@zdicesare zdicesare commented May 15, 2018

Jaeger containers were changed to no longer require the path
to the executable as an argument to docker run, so remove all
instances of executable path

Signed-off-by: Zachary DiCesare zachary.v.dicesare@gmail.com

@zdicesare zdicesare force-pushed the feature/dockerfile-entrypoint branch from 9dcc5f0 to 85473ca Compare May 15, 2018 23:33
@jpkrohling jpkrohling changed the title Remove executable as an argument to containers WIP - Remove executable as an argument to containers May 16, 2018
@@ -36,7 +36,6 @@ items:
- image: jaegertracing/jaeger-collector:1.2
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This image is still not following the changes from jaegertracing/jaeger#815. You'll need a new version of those images and then change the YAML files here to use this new version.

@jpkrohling
Copy link
Copy Markdown
Collaborator

jpkrohling commented May 16, 2018

I'm adding the WIP tag to this PR, as there are tasks that need to be completed before this change:

@zdicesare
Copy link
Copy Markdown
Contributor Author

Thanks for the patience here @jpkrohling :)

Jaeger containers were changed to no longer require the path
to the executable as an argument to docker run, so remove all
instances of executable path

Signed-off-by: Zachary DiCesare <zachary.v.dicesare@gmail.com>
@zdicesare zdicesare force-pushed the feature/dockerfile-entrypoint branch from 85473ca to 5cb25e4 Compare July 9, 2018 22:46
Signed-off-by: Zachary DiCesare <zachary.v.dicesare@gmail.com>
@zdicesare zdicesare force-pushed the feature/dockerfile-entrypoint branch from 5cb25e4 to 8e6c6e8 Compare July 9, 2018 22:47
@zdicesare
Copy link
Copy Markdown
Contributor Author

The build will fail until 1.6.0 images are pushed

@zdicesare
Copy link
Copy Markdown
Contributor Author

I have a local branch bumping the version of https://github.com/kubernetes/charts/tree/master/incubator/jaeger

However, since parameters are specified there via environment variables, I don't think a version bump will have an effect w/ the Dockerfile change, right? I could still bump the version anyways (currently it's at 1.4.1) if we want to update it.

@jpkrohling
Copy link
Copy Markdown
Collaborator

@jkandasa would you be able to test this PR this sprint?

Copy link
Copy Markdown
Collaborator

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I could test this and it failed locally. I got it working with the following changes:

https://gist.github.com/jpkrohling/75250433d508c53b407680fc9c67e547

@zdicesare, would you please update this PR to include the changes from the gist above? Once that's done, it's ready to be merged.

Needed now that Dockerfiles use ENTRYPOINT

Signed-off-by: Zachary DiCesare <zachary.v.dicesare@gmail.com>
These are now packaged in the binary

Signed-off-by: Zachary DiCesare <zachary.v.dicesare@gmail.com>
@zdicesare
Copy link
Copy Markdown
Contributor Author

Yikes, my apologies @jpkrohling. First experience with k8s and openshift, but this was sloppy.

@zdicesare
Copy link
Copy Markdown
Contributor Author

It looks like these Travis failures are occurring on master as well

@jpkrohling jpkrohling changed the title WIP - Remove executable as an argument to containers Remove executable as an argument to containers Jul 12, 2018
@jpkrohling jpkrohling merged commit e49ee50 into jaegertracing:master Jul 12, 2018
@jpkrohling
Copy link
Copy Markdown
Collaborator

Thank you for your contribution, @zdicesare!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants