Skip to content
This repository was archived by the owner on Nov 26, 2025. It is now read-only.

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

Merged
james-garner-canonical merged 10 commits intocanonical:mainfrom
imanenami:dpe-6468
Nov 25, 2025
Merged

[DPE-6468] feat: add connect_client data interface#226
james-garner-canonical merged 10 commits intocanonical:mainfrom
imanenami:dpe-6468

Conversation

@imanenami
Copy link
Copy Markdown
Contributor

Changes:

This pull request introduces connect_client interface adhering to the __template__ interface definition.

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.

It looks appropriate on the business logic, although we should probably add two things:

  1. Add which fields are passed as secrets
  2. Add the optional fields for tls, as for the other client interfaces (e.g. kafka, mysql, etc)

You can probably take some inspiration from zookeeper interface

But then it would probably be sensible for either @jnsgruk, @simskij or @gruyaume to give a +1

@imanenami
Copy link
Copy Markdown
Contributor Author

Thanks @deusebio, I've applied the changes to adapt this interface to the secrets exchange logic we have already implemented in data_interfaces, also added the tls optional field.

@simskij
Copy link
Copy Markdown
Member

simskij commented Jun 19, 2025

@imanenami sorry for leaving this pr hanging for so long. is this still something you are looking to get merged?

@imanenami
Copy link
Copy Markdown
Contributor Author

Hi @simskij, yes I would appreciate if it could get merged. Thanks!

Copy link
Copy Markdown
Contributor

@james-garner-canonical james-garner-canonical left a comment

Choose a reason for hiding this comment

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

Apologies for the massive delay in reviewing this PR.

The only thing I'd really like to see before merging is: if there's a library for this interface, please mention it in the readme -- if there isn't, mention that instead, e.g. "there is no library for this interface, charms should implement the provider/requirer logic directly".

I'm assuming that this interface is already in use, otherwise I would suggest using a JSON serialized list for ConnectProviderData.endpoints, and making ConnectRequirerData optional (or use None / null as the sentinel value).

It would be great to see interface tests added (readme, example) for this interface. No need to block merging this PR for them though, they can be done separately -- but if you want to add them in this PR that's definitely fine.

@james-garner-canonical
Copy link
Copy Markdown
Contributor

Hi @imanenami, we're moving towards archiving this repo very soon, however interface specifications live on in the charmlibs repo. I'm happy to merge this PR and migrate the interface definition to charmlibs, but following my previous comment:

The only thing I'd really like to see before merging is: if there's a library for this interface, please mention it in the readme -- if there isn't, mention that instead, e.g. "there is no library for this interface, charms should implement the provider/requirer logic directly".

@james-garner-canonical
Copy link
Copy Markdown
Contributor

Looks like you'll need to run tox -e build-json-schemas to update the index.json file with the interface information -- I think that piece of CI was added some time after the initial creation of this PR.

@james-garner-canonical
Copy link
Copy Markdown
Contributor

Thanks @imanenami!

@james-garner-canonical james-garner-canonical merged commit f6db59c into canonical:main Nov 25, 2025
6 checks passed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants