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

[stable/locust]: Updates locust chart configmap files glog statement #636

Merged

Conversation

byronmansfield
Copy link
Contributor

@byronmansfield byronmansfield commented Nov 27, 2024

Description

Updates the locustfile's configmap Files Glob syntax. Seems to be a small syntax change for helm 3 maybe.

Checklist

  • Title of the PR starts with chart name (e.g. [stable/mychartname])
  • I have read the contribution instructions, bumped chart version and regenerated the docs
  • Github actions are passing

@byronmansfield byronmansfield requested a review from a team as a code owner November 27, 2024 21:29
@byronmansfield
Copy link
Contributor Author

Hey @max-rocket-internet. So with my last update I was only testing it with helm template locust -n locust -f values.yaml testing.values.yaml --version 0.32.1 --debug . > debug.yaml and the locustfile configmap rendered the data just fine, as expected. With the full python file in there. Now in my testing.values.yaml I was testing with trying to use stable/locust/locustfiles/foo/main.py. And had updated this in the values file according to loadtest.name: foo and loadtest.locust_locustfile: main.py.

However, today I went to actually deploy that latest version (0.32.1) at work. And it did not work. I spent some time troubleshooting it. At work I cannot run helm template because that is only for local charts. I got home and troubleshot it locally with helm upgrade -n locust --dry-run -n locust -f values.yaml -f test.values.yaml --version 0.32.1 oci://ghcr.io/deliveryhero/helm-charts/locust > debug.yaml and sure enough, it was still not rendering like helm template ... does. Odd.

So I eventually got it working like this and instead of using the remote one like my helm upgrade above. I just switched it out for . (locally). It seems to work this way.

Let me know your thoughts and if it needs more testing. Sorry I didn't catch this before with helm template. It's odd to me (and I'm not that experienced with contributing to helm charts) that helm template and helm upgrade --dry-run render two different things for the locustfile configmap.

I hope all my comment made sense.

@max-rocket-internet
Copy link
Member

Hey @byronmansfield, I don't have time to do thorough reviews of all these PRs...

The configmap created by stable/locust/templates/configmap-locust-locustfile.yaml is just an example to show how it works. i.e. the format of the configmap and how it's mounted inside the locust pod. This configmap cannot be used if you are using your own locustfile (as you should be) because helm can only access files within the chart itself. This is stated already in the readme:

By default it will install using an example locustfile and lib from stable/locust/locustfiles/example. When you want to provide your own locustfile, you will need to create 2 configmaps using the structure from that example

So I think just revert stable/locust/templates/configmap-locust-locustfile.yaml back to the version before your #634 PR.

Otherwise this configmap is created for no reason in most cases, right?

@byronmansfield
Copy link
Contributor Author

Ahhh.... sorry I missed that. Ok, that makes sense. I guess I was hoping that I could do both in one sweep so I didn't have to add an additional step. And thank you for all your time spent on my reviews. I know I have been sending you a lot recently. Apologies for that.

As for reverting #634 would you like me to open a new PR? Or just update this one?

@max-rocket-internet
Copy link
Member

I guess I was hoping that I could do both in one sweep so I didn't have to add an additional step

Understandable. I also tried some years back but wouldn't find an elegant solution.

As for reverting #634 would you like me to open a new PR? Or just update this one?

As you wish.

Maybe you could look into helm --set-file option. It looks like it's for passing file contents into a helm chart: https://helm.sh/docs/helm/helm_install/

@byronmansfield
Copy link
Contributor Author

I hope just adding back the old conditionals in this PR is ok. And thanks for the --set-file advice. That is a pretty cool feature. I actually never knew about this. That might actually do the trick.

Copy link
Member

@max-rocket-internet max-rocket-internet left a comment

Choose a reason for hiding this comment

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

All good, thanks @byronmansfield 💖

@max-rocket-internet max-rocket-internet merged commit b9b34cd into deliveryhero:master Dec 9, 2024
7 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
Development

Successfully merging this pull request may close these issues.

2 participants