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] Add support for git sync #641

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

AlexisBRENON
Copy link

Description

Add Git Sync support to locust chart to easily deploy multiple locustfiles.

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

@AlexisBRENON AlexisBRENON requested a review from a team as a code owner December 16, 2024 10:27
@max-rocket-internet
Copy link
Member

Sorry @AlexisBRENON but this is a huge increase in complexity, with no description of why and no docs for how a chart user might make use of this feature.

@max-rocket-internet
Copy link
Member

How about a more generic approach that moves all this complexity to the values provided to the chart (and hence the person wanting to do this):

  • Add a .Values.initContainers option
  • Add a .Values.volumes option

This would be very simple and also, AFAIU, would allow you to achieve the same thing?

@AlexisBRENON
Copy link
Author

Thanks for the feedback. I agree, I didn't take time to document the new features, maybe I opened the PR a little bit early (moreover, the template is not rendering for the moment...).

Using a more generic approach report the complexity on the user of the chart. Moreover, it also requires a .Values.sidecars to allow for the git sync sidecar... Once documented, you will see that the use is quiet straight-forward, allowing the user to use any Git repository as the source of multiple locustfiles (as well as libraries, requirements file, etc.).
I tried to not break the current compatibility.

For information, I took inspiration from the Apache Airflow chart which can also relies on Git Sync to provide DAG files to the different components.

Are you totally againt such solution (in which case I can close the PR) or would you let me improve it a little bit (at least adding the documentation) (in which case I will ping you back when it's done) ? Or I can close the current PR and open a new one when I'm ready.

@max-rocket-internet
Copy link
Member

I'm on holiday right now, I'll revisit it when I get back in Jan, but in principle if there's a bunch of people that want this feature then all good, of course we can fix the issues and merge. But it looks like quite an advanced use case and from what I've seen so far, from the locust repo issues and the locust Slack org, even just using the helm chart as it is now can be a challenge.

If anyone else has strong opinions on this then feel free to chime in 😃

Happy holidays @AlexisBRENON 🎄

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