Skip to content

[DPE-6468] feat: add connect_client data interface#205

Merged
imanenami merged 9 commits intomainfrom
feat/dpe-6468
Jul 30, 2025
Merged

[DPE-6468] feat: add connect_client data interface#205
imanenami merged 9 commits intomainfrom
feat/dpe-6468

Conversation

@imanenami
Copy link
Copy Markdown
Contributor

@imanenami imanenami commented Feb 10, 2025

Changes:

  • Added connect_client requirer and provider data interfaces
  • Added kafka-connect-charm test charm with an implementation of KafkaConnectProvides
  • Added KafkaConnectRequires to application-charm

Copy link
Copy Markdown
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

The changes are fine. I'm just wondering if we should raise a PR to the charm-relation-interface before merging this?

Copy link
Copy Markdown
Contributor

@taurus-forever taurus-forever left a comment

Choose a reason for hiding this comment

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

LGTM for the code. +1 to Dragomir: AFAIK, all new interfaces need description on https://github.com/canonical/charm-relation-interfaces/ (c) Mykola.
The final decision here in on @deusebio .

@imanenami
Copy link
Copy Markdown
Contributor Author

imanenami commented Mar 10, 2025

Thanks, PR for charm-relation-interfaces opened here: canonical/charm-relation-interfaces#226

@imanenami imanenami requested a review from deusebio March 10, 2025 07:03
@deusebio deusebio removed the request for review from welpaolo March 10, 2025 09:23
@deusebio
Copy link
Copy Markdown
Contributor

Yes, as I mentioned in my comment earlier, we should land the charm-relation-interface PR first. I have already provided some comments there. Please have a look

Copy link
Copy Markdown
Contributor

@deusebio deusebio left a comment

Choose a reason for hiding this comment

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

Anyhow, the code here is totally fine! Just wait for the other PR to land to make sure that there are no comments there to be addressed in the implementation here. Thanks!

@imanenami imanenami force-pushed the feat/dpe-6468 branch 2 times, most recently from 40ed0ab to 7c82de2 Compare July 30, 2025 10:38
Copy link
Copy Markdown
Contributor

@sinclert-canonical sinclert-canonical left a comment

Choose a reason for hiding this comment

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

I was not aware of this effort from 5 months ago, but it seems to be in good shape.

I noticed that the KafkaConnect family of classes lack the support of custom roles that got recently introduced in all other products (MongoDB, MySQL, PostgreSQL, OpenSearch, Kafka & Karapace). Is this a deliberate decision?

  • If so, could you share why? AFAIK, Mykola wanted custom roles support product wide.
  • If not, please take a look at how Marc introduced the custom roles support to the Karapace classes, once they were already defined on a previous PR. You can use it as a reference: #238

Disclaimer:

Please be sure I will be extraordinary fast to approve this PR once my concern gets clarified (either way). I just want to get an answer in case the custom-roles effort got overlooked. The last thing I want to do is to block a 5 months ago effort, being added as a reviewer last minute 😅

Comment thread lib/charms/data_platform_libs/v0/data_interfaces.py Outdated
Copy link
Copy Markdown
Contributor

@sinclert-canonical sinclert-canonical left a comment

Choose a reason for hiding this comment

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

Clarified via MatterMost: ACLS and permissions are not inherently supported by KafkaConnect.

Co-authored-by: Sinclert Pérez <sinclert.perez@canonical.com>
@imanenami imanenami merged commit bb180d6 into main Jul 30, 2025
61 of 64 checks passed
@imanenami imanenami deleted the feat/dpe-6468 branch July 30, 2025 13:23
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.

5 participants