-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
feat: samples for Confluent operator tutorial #725
Conversation
Here is the summary of changes. You are about to add 10 region tags.
This comment is generated by snippet-bot.
|
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.
Issue:
The /streaming/kafka-confluent/terraform
folder and its contents that we're adding in this pull-request is mostly identical to the existing /streaming/kafka-strimzi/terraform
folder and its contents. Could we please reduce this duplication (all or most)? For instance, could we reuse the existing /streaming/kafka-strimzi/terraform/
directory and consider moving /streaming/kafka-confluent/terraform/
to a new folder /streaming/kafka/terraform/
(so that it's clear that it's not Strimzi-specific)?
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.
Well, the only difference in tf code is terraform tags (could be the same, it doesn't matter), the "component" label for the cluster (can fix with component = "strimzi-operator" -> component = "${var.cluster_prefix}-operator") and ingress rule for replicator (different ports, can add both in the rule to work with any operator)
@aburhan @NimJay your opinion?
Also these changes will affect the documentation, so i will need to fix Basic and Replication guides
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.
@ganochenkodg I think its a good idea to pull out the terraform into its own folder as @NimJay mentioned since the code is identical (/streaming/kafka/terraform/). The manifest files for each are different so we can have operator specific folders for those manifests under streaming/kafka/confluent. I work later to rework the strimzi files
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.
Then i'll create an additional PR with only these changes and let you know
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, you two, for working toward removing Terraform duplication.
As discussed with @aburhan, we can merge this pull-request as is (without the Terraform refactoring).
I've opened #861 to track the duplication issue.
@ganochenkodg if you want, you can just move the /streaming/kafka-confluent/terraform/
folder to /streaming/kafka/terraform/
, and we can worry about updating the Strimzi tutorial later. Or merge as is.
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.
Hi @ganochenkodg,
Thank you so much for these samples.
(And sorry about the late review!)
I just have two immediate concerns/questions:
- For Kafka/Confluent-related issues, do we have a Googler that can debug any issues/bugs we'll encounter in these samples (in the future)?
- See my other comment about reducing Terraform duplication.
I have not tested these samples myself, but I trust you or the tech writers (or another sample author) have/will do the appropriate testing before publishing these samples.
Thanks again! :)
Co-authored-by: Nim Jayawardena <[email protected]>
What do you think? |
Note: If the |
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 again, @ganochenkodg and @aburhan, for your patience here (and for creating these samples!).
Approved. Feel free to merge.
No description provided.