-
Notifications
You must be signed in to change notification settings - Fork 4
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
Zbchaos: Support SaaS setup #163
Conversation
@Zelldon I'm not sure that I'm the right person to review this. I'm not so familiar with Go code, and have no clue how this zbchaos cli works. Perhaps you guide me through this code once, but it might be better for someone else to review this now. |
I thought you were interested in the feature :) don't think that you need deep knowledge here :D but if you don't want to do it it is also ok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Looks good!
Very soft suggestions: I think we could extend this to simply have a set of pre-defined labels you can pick either via CLI or env var, and a flag/env var where you can define your own labels, instead of a fallback logic.
e.g.
zbchaos --labels "app.kubernetes.io/app=zeebe" --labels "app.kubernetes.io/component=myCustomComponent" doMySuperCustomThing
zbchaos --profile camunda-cloud doCloudThing
zbchaos --profile csm doSelfManagedThing
zbchaos doSelfManagedThing
I think having a fallback logic is a bit too opaque for most users. Having specific flags to specify the labels lets you document the logic in the help/usage text directly. That said, if we're the expected audience then it's probably fine 😄
Hey @npepinpe thank you very much for the fast review! 💪 I also thought about this and the first idea was to have default self-managed and support a flag for SaaS, but tbh I thought it would be much nicer and easier to use if the tool can just do it. I mean we already know the details and I felt it would be a nicer experience (ux). Especially since I normally don't care about such details in regards to the labels. Plus it feels sometimes so overwhelming If I have to specify so many parameters :D Since you mentioned it is a soft suggestion I would leave as it is if you're ok with it. We can of course always revisit if we found out it was a bad idea :) |
We will discover in time if it's an issue, so just go ahead and merge 😄 I think it's mostly an issue of discoverability, i.e. this behavior is not obvious to others from the outside and hard to find out unless you read the code or someone tells you, whereas using flags you just do |
Previously the zbchaos cli just supported the self-managed helm setup, with this PR we add support for SaaS. This is done via using different labels, which can also be found in our current used scripts https://github.com/zeebe-io/zeebe-chaos/blob/main/chaos-workers/chaos-experiments/scripts/utils.sh#L17-L35
In order to reduce the burden of the user to set specific flags for certain environment I implemented it in a way that multiple environments are checked.
With that change we can now finally run
zbchaos topology
in ultrachaosFurthermore I create more unit tests with a fake k8 client, this allows us to verify whether the getBrokerPods etc. work correctly.
@korthout no worries most of the code is test code, the changes are fairly simple it is enough to have a rough scan :)