-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[Stretch Cluster] Add stretch cluster feature gate #11336
base: main
Are you sure you want to change the base?
[Stretch Cluster] Add stretch cluster feature gate #11336
Conversation
- Introduced a 'useStretchCluster' feature gate Signed-off-by: Rohan Anil Kumar <[email protected]>
8639b43
to
07b4322
Compare
fix variable name camel case convention Signed-off-by: Rohan Anil Kumar <[email protected]>
Hi, @rohan-anil-kumar — thanks a lot for your contribution and interest in working on this! Just to give a bit of guidance: The proposal for the Stretch Cluster feature hasn’t been approved yet, so introducing a FEATURE_GATE at this stage (without any accompanying logic or PoC) might be a bit premature. Typically, we prefer to see some initial implementation or PoC along with the feature gate to better understand its purpose and how it fits into the broader design. Would you be open to first submitting a more comprehensive PR — perhaps with some basic logic or a minimal PoC — so we can better evaluate it? |
Hi @see-quick, thank you for taking the time to review this PR. |
Thank you so much for the guidance and quick response! |
We can also create a branch and you can opening smaller PRs against the branch. It will be easier to review smaller changes and it will also does not "polute" main branch witch code parts that are kinda not used at all. Then, at some point, the branch will contain whole feature mostly reviewed and it could be merged. Like as we are doing with STs refactor - #11303 for reference. |
So @aswinayyolath pinged me offline and I was ok with this approach but maybe what we can do is leveraging the @Frawless approach by going with simpler PRs but through a branch which is not the main. Of course the same apply to #11335 by @aswinayyolath PS: @rohan-anil-kumar I see you tried to ping me and Kate but using wrong tags :-D. So you actually tagged other people. |
Hi @rohan-anil-kumar and @aswinayyolath, I do think it's good for the implementation to be delivered in smaller PRs if we can break it down in a way that makes sense. However I have two concerns about this PR:
|
Thank you all for the detailed feedback and guidance — really appreciate the direction on how best to proceed 🙏 Just to confirm my understanding:
In the meantime, we’ll ensure the branch stays up-to-date with main and resolve any conflicts as needed. Since contributors don’t have permission to create branches in the Strimzi repo, would it be possible for one of the maintainers to create the feature branch for us? We’re happy to start raising PRs against it right away. Also we'll focus on getting the proposal aligned and approved first.. Thanks again for your support! |
I do not think there should be any merging into any Strimzi branches until the proposal is approved. Once it is approved, it is definitely possible. But keep in mind that there will be still one big PR that needs to be reviewed in order to merge the feature branch into the main branch. |
Type of change
Select the type of your PR
Description
This PR enables a feature gate called UseStretchCluster to enable stretch cluster on the strimzi operator.
Checklist
Please go through this checklist and make sure all applicable tasks have been done
Signed-off-by: Rohan Anil Kumar [email protected]