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

Add a way to configure extra receivers for uncharmed applications #7

Merged
merged 4 commits into from
Jul 1, 2024

Conversation

michaeldmitry
Copy link
Contributor

@michaeldmitry michaeldmitry commented Jun 26, 2024

Issue

Fixes canonical/tempo-k8s-operator#87

Solution

Expose juju config options to force enable each protocol supported by Tempo.

Testing instructions

Deploy Tempo coordinator

cwd to tempo-coordinator-k8s-operator

charmcraft pack
juju deploy ./tempo-coordinator-k8s_ubuntu-22.04-amd64.charm 

"Unblock" coordinator

juju deploy facade s3-facade --channel=latest/edge
juju integrate s3-facade:provide-s3 tempo-coordinator-k8s:s3
juju run s3-facade/0 update --params ./tests/manual/facades/s3.yaml

cwd to tempo-worker-k8s-operator

git fetch
git checkout coordinator-integration
charmcraft pack
juju deploy ./tempo-worker-k8s_ubuntu-22.04-amd64.charm  tempo-worker --resource tempo-image=docker.io/ubuntu/tempo:2-22.04
 juju integrate tempo-coordinator-k8s tempo-worker

Validation

juju run tempo-coordinator-k8s/0 list-receivers

should see sth like

otlp-http: http://coord-0.coord-endpoints.test.svc.cluster.local:4318

Enable extra receiver

juju config tempo-coordinator-k8s always_enable_zipkin=True
juju run tempo-coordinator-k8s/0 list-receivers

Should now see sth like

otlp-http: http://coord-0.coord-endpoints.test.svc.cluster.local:4318
zipkin: http://coord-0.coord-endpoints.test.svc.cluster.local:9411

Copy link
Contributor

@PietroPasotti PietroPasotti left a comment

Choose a reason for hiding this comment

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

looking good!
not sure why this PR and the other both share a bunch of apparently overlapping changes to the test code (such as the commented-out modules).
+100 for fixing the tests, but still I'd have preferred to have a single pr to fix the tests, then two PRs on top of that doing the receivers and the pydantic port.

This makes it quite hard to review the test code changes.
For the next time :)
Now let's try to merge these two.

src/charm.py Show resolved Hide resolved
src/tempo.py Show resolved Hide resolved
tests/scenario/test_enabled_receivers.py Outdated Show resolved Hide resolved
tests/scenario/test_enabled_receivers.py Outdated Show resolved Hide resolved
charmcraft.yaml Show resolved Hide resolved
charmcraft.yaml Outdated Show resolved Hide resolved
@michaeldmitry
Copy link
Contributor Author

looking good! not sure why this PR and the other both share a bunch of apparently overlapping changes to the test code (such as the commented-out modules). +100 for fixing the tests, but still I'd have preferred to have a single pr to fix the tests, then two PRs on top of that doing the receivers and the pydantic port.

This makes it quite hard to review the test code changes. For the next time :) Now let's try to merge these two.

100% agreed. Will take care next time.
Comments on the overlapping changes for test files will be addressed in #5

charmcraft.yaml Show resolved Hide resolved
charmcraft.yaml Show resolved Hide resolved
@michaeldmitry michaeldmitry merged commit 7d1d957 into main Jul 1, 2024
13 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
3 participants