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

Allow overriding k8s namespace #193

Merged
merged 9 commits into from
Oct 14, 2022
Merged

Allow overriding k8s namespace #193

merged 9 commits into from
Oct 14, 2022

Conversation

lenaschoenburg
Copy link
Contributor

This adds a flag similar to kubeconfig to override the namespace that will be used by the k8s client.

Depends on #192

Copy link
Member

@ChrisKujawa ChrisKujawa left a comment

Choose a reason for hiding this comment

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

Thanks 🙇 One remark :)

//// based on https://github.com/kubernetes/client-go/blob/master/examples/out-of-cluster-client-configuration/main.go
var kubeconfig *string
if home := homedir.HomeDir(); home != "" {
kubeconfig = flag.String("kubeconfig", filepath.Join(home, ".kube", "config"), "(optional) absolute path to the kubeconfig file")
} else {
kubeconfig = flag.String("kubeconfig", "", "absolute path to the kubeconfig file")
}
namespace := flag.String("namespace", "", "Kubernetes namespace to use")
Copy link
Member

Choose a reason for hiding this comment

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

❌ This is a bit hidden. I would like to use a presistent flag https://stackoverflow.com/a/63498490/2165134 as we do with verbose here https://github.com/zeebe-io/zeebe-chaos/blob/main/go-chaos/cmd/root.go#L53

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's what I tried first but that introduced a circular dependency from cmd -> internal -> cmd again. I guess that's why the kubeconfig parameter is also defined here and not in cmd?

Copy link
Member

Choose a reason for hiding this comment

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

well what you need to do is then to set a internal property like we do for verbosity see here https://github.com/zeebe-io/zeebe-chaos/blob/main/go-chaos/cmd/root.go#L48

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, neat 👍 Do you remember if there was a reason to not do the same for the kubeconfig param?

Comment on lines +52 to +53
internal.Namespace = Namespace
internal.KubeConfigPath = KubeConfigPath
Copy link
Member

Choose a reason for hiding this comment

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

I guess we need also some defaults ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ChrisKujawa
Copy link
Member

works thanks @oleschoenburg 🚀

[zell go-chaos/ cluster: zeebe-cluster ns:medic-cw-41-966dac308f-benchmark]$ ./zbchaos topology -n medic-cw-41-966dac308f-benchmark-mixed
Node      |Partition 1         |Partition 2         |Partition 3
2         |FOLLOWER (HEALTHY)  |FOLLOWER (HEALTHY)  |FOLLOWER (HEALTHY)
0         |LEADER (HEALTHY)    |FOLLOWER (HEALTHY)  |LEADER (HEALTHY)
1         |FOLLOWER (HEALTHY)  |LEADER (HEALTHY)    |FOLLOWER (HEALTHY)
[zell go-chaos/ cluster: zeebe-cluster ns:medic-cw-41-966dac308f-benchmark]$ ./zbchaos topology
Node      |Partition 1         |Partition 2         |Partition 3
1         |FOLLOWER (HEALTHY)  |LEADER (HEALTHY)    |FOLLOWER (HEALTHY)
2         |FOLLOWER (HEALTHY)  |FOLLOWER (HEALTHY)  |FOLLOWER (HEALTHY)
0         |LEADER (HEALTHY)    |FOLLOWER (HEALTHY)  |LEADER (HEALTHY)

@ChrisKujawa ChrisKujawa merged commit baa7499 into main Oct 14, 2022
@ChrisKujawa ChrisKujawa deleted the os-zbchaos-namespace branch October 14, 2022 05:28
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