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.

Sign up for GitHub

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

Jump to bottom

Move connect configuration set up to the operator #11062

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Move connect configuration set up to the operator #11062

wants to merge 6 commits into from

Conversation

tinaselenge
Copy link
Contributor

Type of change

  • Refactoring

Description

Generates a config map containing Kafka Connect configurations instead of setting them up using shell script.

Addresses part of #10668.

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

@tinaselenge tinaselenge force-pushed the connect-config branch 4 times, most recently from 33fada7 to 35ce42e Compare January 22, 2025 15:29
@tinaselenge tinaselenge marked this pull request as ready for review January 23, 2025 09:17
@ppatierno ppatierno added this to the 0.46.0 milestone Jan 23, 2025
@tinaselenge tinaselenge force-pushed the connect-config branch 3 times, most recently from 76e7c40 to 2e90d3a Compare January 28, 2025 16:16
scholzj
scholzj reviewed Jan 29, 2025
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

@tinaselenge The code looks good. But the unit test failures seem to be related to this change. Can you please have a look at it? Thanks.

@tinaselenge
Address review comments
b905456 
Signed-off-by: Gantigmaa Selenge <[email protected]>
@scholzj
Copy link
Member

scholzj commented Jan 29, 2025

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Jan 29, 2025

/azp run upgrade

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Jan 30, 2025

FYI: I can restart the regression if needed. But the failures seem all related to Connect / MM2.

@tinaselenge tinaselenge force-pushed the connect-config branch 4 times, most recently from cbcd7d5 to 9cd1241 Compare January 30, 2025 12:33
@ppatierno
Copy link
Member

@tinaselenge now it's the build to fail and it's related to tests for this PR AFAICS.

@tinaselenge
Copy link
Contributor Author

@ppatierno @scholzj thanks. Yes I have been working to fix the tests.

@tinaselenge
Fix unit and system tests
b68548d 
Signed-off-by: Gantigmaa Selenge <[email protected]>
@tinaselenge
Copy link
Contributor Author

@scholzj can you please kick off the regression tests again?

I need to look into RackAwarenessST again as it previously read consumer.client.rack from strimzi-connect.properties file by exec-ing into the pod. However, this config value is now provided through config provider. I will look into how/if we can make this test check the actual value. For now, this test should still pass.

@ppatierno
Copy link
Member

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tinaselenge
Fix RackAwarenessST failure
f6894b2 
Signed-off-by: Gantigmaa Selenge <[email protected]>
@YuanShisong YuanShisong mentioned this pull request Feb 11, 2025
Open
@tinaselenge
Copy link
Contributor Author

@scholzj @ppatierno can one of you please kick off the regression tests again? I believe I fixed the ones that failed before. Thank you

@scholzj
Copy link
Member

scholzj commented Feb 11, 2025

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@scholzj
Copy link
Member

scholzj commented Feb 12, 2025

/azp run regression

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

katheris
katheris reviewed Feb 12, 2025
View reviewed changes
Copy link
Contributor

@katheris katheris left a comment

Choose a reason for hiding this comment

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

Added a bunch of comments, all nits around method naming and consistent javadoc. Particularly in the new class I think we should try to be consistent so it's easier to navigate, otherwise it looks good to me

...erator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectConfigurationBuilder.java
}

/**
* Prints the file header which is on the beginning of the configuration file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Prints the file header which is on the beginning of the configuration file.
* Internal method which prints the file header which is at the beginning of the configuration file.

Aligning the Javadoc with elsewhere where we explicitly declare which methods are internal. I don't think we necessarily need to explicitly state they are internal since the they are private methods, but we should be consistent in the Javadoc

...erator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectConfigurationBuilder.java Outdated Show resolved Hide resolved
...erator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectConfigurationBuilder.java Outdated Show resolved Hide resolved
...erator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectConfigurationBuilder.java Outdated Show resolved Hide resolved
cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaConnectClusterTest.java Outdated Show resolved Hide resolved
cluster-operator/src/test/java/io/strimzi/operator/cluster/model/KafkaConnectClusterTest.java Outdated Show resolved Hide resolved
@scholzj
Copy link
Member

scholzj commented Feb 13, 2025

/azp run upgrade

@azure-pipelines Azure Pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

scholzj
scholzj approved these changes Feb 13, 2025
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

One nt. But LGTM otherwise. Thanks.

...erator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectConfigurationBuilder.java Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppatierno ppatierno ppatierno left review comments

@katheris katheris katheris left review comments

@scholzj scholzj scholzj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.46.0
Development

Successfully merging this pull request may close these issues.
4 participants
@tinaselenge @scholzj @ppatierno @katheris