-
Notifications
You must be signed in to change notification settings - Fork 314
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
Fix multi-cluster attribute check for wrapped runners #1563
Conversation
… attribute conditional bug
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.
Thanks! I'm wondering about the general direction here. Looking at #488 it looks like multi-cluster support was meant for custom runners that wanted to do different operations for each cluster.
Here you want to do the same operation for all clusters, which begs the following questions:
- What if you don't actually want to delete data streams in all clusters?
- This does not generalize very well: are we going to add 100 lines of code every time we want an operation to work on all clusters?
It seems to me that we can address both those concerns by adding a new operation say all-clusters
that will run a given operation on all clusters. Maybe I'm over thinking it: I'd like to know what @dliappis thinks, as he added #488.
(Also, if this was split in two commits or two PRs, we could have merged the first part! It's always suspicious when a PR or commit contains the word "and".)
…-cluster attribute conditional bug" This reverts commit b96f0c4.
I agree with these comments, and therefore reverted the
Agreed and apologies, I kept the 'bug' fix in 287fbc0. I've renamed the PR title to match. |
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.
Thanks! LGTM. I left two small comments, feel free to merge after addressing them.
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.
Thanks! LGTM.
With this commit we fix an existing bug where the multi-cluster attribute of
wrapped runners were not checked properly.