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

✨ Added jaeger to images so it runs out of the box #297

Merged
merged 5 commits into from
Aug 22, 2023

Conversation

Parthiba-Hazra
Copy link
Contributor

Fixes: #236

@Parthiba-Hazra Parthiba-Hazra changed the title ✨ Added jaeger to images so it runs out of the box ✨ Added jaeger to images so it runs out of the box Aug 18, 2023
@pranavgaikwad
Copy link
Contributor

@Parthiba-Hazra Thank you for your PR! Can we not build Jaeger and directly include the jaeger/all-in-one image? Also, can we remove duplication in demo.Dockerfile and use the binary directly from base layer?

@Parthiba-Hazra
Copy link
Contributor Author

Parthiba-Hazra commented Aug 21, 2023

@Parthiba-Hazra Thank you for your PR! Can we not build Jaeger and directly include the jaeger/all-in-one image? Also, can we remove duplication in demo.Dockerfile and use the binary directly from base layer?

@pranavgaikwad actually I tried that. I was getting some problem to start the jaeger .. will try that again. I will reach out to you if I need any help. Thank you :)

@Parthiba-Hazra
Copy link
Contributor Author

@pranavgaikwad please review the changes.. sorry for the trouble actually I had to use command all-in-one-linux instead of jaeger-all-in-one. :)

demo.Dockerfile Outdated Show resolved Hide resolved
demo.Dockerfile Outdated Show resolved Hide resolved
Copy link
Contributor

@pranavgaikwad pranavgaikwad left a comment

Choose a reason for hiding this comment

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

Can we also enable jaeger by default? instead of having users to pass it, now that it's already in the image, it should just work

demo.Dockerfile Outdated Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
@pranavgaikwad
Copy link
Contributor

@Parthiba-Hazra thank you! appreciate your patience on this. Going to to give this a quick test before merging.

@pranavgaikwad
Copy link
Contributor

@shawn-hurley any objection on using jaeger image from docker.io?

@shawn-hurley
Copy link
Contributor

None from me, I am a plus one

@pranavgaikwad pranavgaikwad merged commit 6129a8b into konveyor:main Aug 22, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add jaeger to image so it runs out of the box
3 participants