Jaeger v2 with v1 #609
Jaeger v2 with v1 #609
Conversation
charts/jaeger/templates/_helpers.tpl
Outdated
|
|
||
|
|
||
|
|
||
| {{- define "jaeger-v2.name" -}} |
There was a problem hiding this comment.
Why any of these changes below necessary? Doesn't the chart already have them all for v1? Can we not reuse the same variables?
There was a problem hiding this comment.
yes that is what i planed to do once i was sure that i was going in the right direction with v-2 deployment and config part
|
I see queries going to |
hellspawn679
force-pushed
the
jaeger-v2-with-v1
branch
from
221e7ec to
a16865c
Compare
Signed-off-by: Reddysekhar Gaduputi <[email protected]> upgrade jaeger-operator to latest 1.61.0 (jaegertracing#605) Signed-off-by: Blair Bowden <[email protected]> added all-in-one deployment and configmap for jaeger-v2 Signed-off-by: Mehul <[email protected]> lint fix Signed-off-by: Mehul <[email protected]> fixed Signed-off-by: Mehul <[email protected]> lint fix Signed-off-by: Mehul <[email protected]> lint fix Signed-off-by: Mehul <[email protected]> fixed using pre-hook Signed-off-by: Mehul <[email protected]> fixed --config flag is not been passed Signed-off-by: mehul <[email protected]> release ns for config-map.yaml Signed-off-by: mehul <[email protected]> testing ci Signed-off-by: mehul <[email protected]> testing ci Signed-off-by: mehul <[email protected]> fixed ns Signed-off-by: mehul <[email protected]> fixed ns Signed-off-by: mehul <[email protected]> fixed template Signed-off-by: mehul <[email protected]> removed sampling Signed-off-by: mehul <[email protected]> removed adaptive sampling from processors Signed-off-by: mehul <[email protected]> Revert "Jaeger v2 test2" Signed-off-by: mehul <[email protected]> attempt to create v2 chart in v1 Signed-off-by: mehul <[email protected]> enabled collector query and agent Signed-off-by: mehul <[email protected]> version bump Signed-off-by: mehul <[email protected]> testing-v2-ci Signed-off-by: mehul <[email protected]> testing-v2-ci Signed-off-by: mehul <[email protected]> added --helm-extra-set-args flag Signed-off-by: mehul <[email protected]> fixed healthcheck port-v2 Signed-off-by: mehul <[email protected]> Fix health check path Signed-off-by: Yuri Shkuro <[email protected]> minor changes Signed-off-by: mehul <[email protected]>
hellspawn679
force-pushed
the
jaeger-v2-with-v1
branch
from
a16865c to
22c8ade
Compare
Signed-off-by: mehul gautam <[email protected]>
| exporters: [{{ join ", " .Values.service.pipelines.traces.exporters }}] | ||
|
|
||
| extensions: | ||
| {{- include "jaeger-v2.extensionsConfig" . | nindent 6 }} |
There was a problem hiding this comment.
I recommend not using templates here - all they do is resolve to a single line, it's better for readability to just inline that line here.
charts/jaeger/values.yaml
Outdated
| storage: | ||
| traces: primary_store | ||
| traces_archive: archive_store | ||
|
|
charts/jaeger/values.yaml
Outdated
| jaeger_storage_exporter: | ||
| trace_storage: primary_store | ||
|
|
||
|
|
There was a problem hiding this comment.
| # The following settings apply to Jaeger v1 and partially to Jaeger v2 | |
Signed-off-by: mehul <[email protected]>
…elm-charts into jaeger-v2-with-v1
Signed-off-by: mehul <[email protected]>
Signed-off-by: mehul <[email protected]>
hellspawn679
requested review from
davidvonthenen,
naseemkullah and
pavelnikolov
as code owners
.github/workflows/lint-test.yaml
Outdated
| ct install --config ct.yaml | ||
| elif [ "${{ matrix.version }}" == "v2" ]; then | ||
| ct install --config ct.yaml --helm-extra-set-args "--set v2.enabled=true --set provisionDataStore.cassandra=false --set storage.type=memory --set allInOne.enabled=true --set agent.enabled=false --set collector.enabled=false --set query.enabled=false" | ||
| fi |
There was a problem hiding this comment.
I believe that the chart can have a breaking version and from this PR one we can support only V2. You shouldn't try to support both of them in the same code branch. V1 can be in a separate branch we can only update it in case of critical bugs or security issues but it will not be maintained in the long run. People should at some point switch to V2.
There was a problem hiding this comment.
@pavelnikolov can you please propose a more concrete plan as far as organizing the repo? How will helm know about different branches?
| @@ -3,7 +3,7 @@ appVersion: 1.53.0 | |||
| description: A Jaeger Helm chart for Kubernetes | |||
| name: jaeger | |||
| type: application | |||
| version: 3.3.1 | |||
| version: 3.3.2 | |||
There was a problem hiding this comment.
This should be a major chart version as per the above comment, IMHO.
charts/jaeger/templates/_helpers.tpl
Outdated
| {{/* | ||
| Create image name for v2 image | ||
| */}} | ||
| {{- define "v2.image" -}} |
There was a problem hiding this comment.
again, there shouldn't be any v2 prefixes. There should be only major chart version bump.
| @@ -105,6 +124,10 @@ spec: | |||
| {{- toYaml . | nindent 12 }} | |||
| {{- end }} | |||
| volumeMounts: | |||
| {{- if .Values.v2.enabled }} | |||
There was a problem hiding this comment.
I guess we should assume that v2 is always enabled and the only one we support from now on in the main branch. Then there can be v1 branch respectively.
charts/jaeger/values.yaml
Outdated
| @@ -2,6 +2,66 @@ | |||
| # This is a YAML-formatted file. | |||
| # Jaeger values are grouped by component. Cassandra values override subchart values | |||
|
|
|||
| v2: | |||
There was a problem hiding this comment.
Let's not try to make this branch support both v1 and v2. We already have a chart which support v1, let's focus 100% on v2 from now on.
Signed-off-by: mehul <[email protected]>
Signed-off-by: mehul <[email protected]>
What this PR does
Which issue this PR fixes
Checklist
[jaeger]or[jaeger-operator])