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

Update helm chart to account for changes to loadgenerator #330

Merged
merged 4 commits into from
Aug 31, 2022

Conversation

Mwad22
Copy link
Contributor

@Mwad22 Mwad22 commented Aug 18, 2022

Now that this issue in opentelemetry-demo repo has been resolved, the helm chart needs to be changed. Specifically, my PR maps the changes in this merged PR to do the following:

  1. account for new environment variables in loadgenerator
  2. deploy load generator as a service (by adding servicePort field) so users can use kubectl port-forward [service] [port] and interact with the locust UI
  3. Generates the specific command to expose Locust UI in NOTES.txt.

NOTE: this can only be merged after new images are built/deployed as current images built the loadgenerator with different environment variables and a --headless tag that disables the locust UI

Re: Testing
I have tested the full functionality following helm chart changes on k8s by generating the template (using helm template) and replacing the image with my own docker image of locust.

@Mwad22 Mwad22 requested a review from a team August 18, 2022 22:54
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Aug 18, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

@dmitryax
Copy link
Member

@Mwad22 please sign the CLA

@Mwad22
Copy link
Contributor Author

Mwad22 commented Aug 19, 2022

@Mwad22 please sign the CLA

Just signed!

@TylerHelmuth
Copy link
Member

@Mwad22 do you know how long it'll be till the new images our available?

@Mwad22
Copy link
Contributor Author

Mwad22 commented Aug 19, 2022

@Mwad22 do you know how long it'll be till the new images our available?

I believe once this PR lands. @austinlparker @puckpuck I believe this was mentioned in the otel-demo slack, but can one of you confirm? thanks!

@TylerHelmuth
Copy link
Member

@Mwad22 still interested in this PR?

@Mwad22
Copy link
Contributor Author

Mwad22 commented Aug 28, 2022

Yes, will update it tomorrow!

@TylerHelmuth
Copy link
Member

@Mwad22 I think there are some other defects affecting the demo chart that are causing your build to fail. I'll review this again when those are fixed in #344

@TylerHelmuth
Copy link
Member

@Mwad22 please bump the chart version

@Mwad22
Copy link
Contributor Author

Mwad22 commented Aug 31, 2022

@Mwad22 please bump the chart version

Just bumped it!

@TylerHelmuth
Copy link
Member

@Mwad22 have you tested this branch with the new official demo images (0.3.4)?

Copy link
Contributor

@Allex1 Allex1 left a comment

Choose a reason for hiding this comment

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

lgtm

@Mwad22
Copy link
Contributor Author

Mwad22 commented Aug 31, 2022

@Mwad22 have you tested this branch with the new official demo images (0.3.4)?

@TylerHelmuth I just tested it again, everything seems to work as expected- with the new Locust UI and everything else!

@TylerHelmuth TylerHelmuth merged commit a6ac672 into open-telemetry:main Aug 31, 2022
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.

4 participants