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

[jaeger-v2]: Fix Kafka E2E Test To Use Default Environment Variables #6027

Closed
mahadzaryab1 opened this issue Sep 28, 2024 · 5 comments · Fixed by #6028
Closed

[jaeger-v2]: Fix Kafka E2E Test To Use Default Environment Variables #6027

mahadzaryab1 opened this issue Sep 28, 2024 · 5 comments · Fixed by #6028

Comments

@mahadzaryab1
Copy link
Collaborator

mahadzaryab1 commented Sep 28, 2024

Problem

@mahadzaryab1
Copy link
Collaborator Author

@yurishkuro Would you be able to help me understand why we needed createConfigWithEncoding? Why could we not just create configuration files that had the desired values that we needed?

@yurishkuro
Copy link
Member

Because we want to test the one official config we provide, not create copies of it that may become out of date.

@mahadzaryab1
Copy link
Collaborator Author

@yurishkuro got it! and does setting the environment variable take precedent over the config in the YAML file?

@yurishkuro
Copy link
Member

It does not. If config has a fixed value the env variable is not going to affect it. Previously you could reference env variable from the config, but then you always have to provide the variable, otherwise the config is invalid. Now you could have both - a default value and a way to override it via env variable.

@mahadzaryab1
Copy link
Collaborator Author

It does not. If config has a fixed value the env variable is not going to affect it. Previously you could reference env variable from the config, but then you always have to provide the variable, otherwise the config is invalid. Now you could have both - a default value and a way to override it via env variable.

@yurishkuro Thanks for the explanation! PR is ready for review at #6028.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

2 participants