Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add all-in-one deployment and configmap for jaeger-v2 #606
Add all-in-one deployment and configmap for jaeger-v2 #606
Changes from 23 commits
8fed143
783ab93
6dba3c9
f858e8a
9439e21
82e4839
9cf70cb
e45d3b5
84e7a07
38d3708
6bc9dcc
be93c14
aac8af9
ae173e5
2bccbc8
0cec95b
144e55f
9aafe0c
12e508f
6985100
0fae1f7
c0fd530
c4ac26b
54a3248
03973fb
e2425eb
2960a50
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
what is the meaning of this version? Is this a reference to Jaeger version? Should it be hardcoded like that?
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 appVersion is only informational according to Helm https://helm.sh/docs/topics/charts/#the-appversion-field but it is used to mark the version of image tag if tag is not set. You can see here (several places) for example query-deploy.yaml calls query.image which uses renderImage which defaults the image to use
Chart.AppVersion
. This ultimately is also used in the bitnami common chart that is referenced on the line below.So YES it should be hardcoded (AND UPDATED!), but it can be removed IF replaced by tag in the
image:
section. But given Jaeger uses a single binary for all deploys, this is a good global place to ensure they are all up to the same version.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 am a bit concerned about starting a jaeger-v2 chart.
Is the plan to deprecate the "jaeger" chart? Would all the template files in the jaeger chart be referenced here or copied over and duplicated?
why not make the templates backwards compatible and just keep the jaeger chart? If you cannot and it is cleaner to start a new one then I would recommend an overhaul and make a common folder to split up the jaeger and jeager-v2 charts then deprecate and eventually remove the jaeger chart and templates that no longer apply.
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.
would this not be version 1.0.0 or 0.1.0 or 1.0.0-rc1 ? Certainly we wouldn't want to start a new chart at 3.3.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.
Upon some review, this has a lot of major regressions compared to the other highly customizable chart. Is this just a draft?
Does version 2 not require a service.yaml and is it not a statefulset?
Service accounts?
Do we not need customization for query, collecter (are they gone in v2 of jeager)?
Nothing for elasticsearch cleaning or indexes or customization?
Oauth?
Spark?
There just seems to be a lot missing here and this is not marked draft or anything
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.
@Stevenpc3 right now this is a proof of concept. I 100% agree with you that there are a lot of moving parts that could be reused from v1 chart, but I don't see a practical path towards that unless existing maintainers in this repo take on that work. This PR is being done by an intern who has both limited knowledge and limited time to get something done to enable Jaeger v2 installs.
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.
Ok. I get it... I know what it's like lol. I didn't mean to seem harsh. I just rely on the Jaeger chart and have some PRs coming and a few contributes already. I was here looking at future updates/plans/paths and got as bit worried. Charts can be fixed, but the churn can have cascading effects. I will help where I can and certainly review. I can get other eyes on it to if needed. I just was thinking it would be published in an incomplete state.
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.
you should probably remove the versions here. Unless using a template like 'helm-doc' to template and generate these versions, they will get out of date and not aligned.
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.
why are you using template instead of include? https://stackoverflow.com/questions/71086697/how-does-template-and-include-differ-in-helm