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.

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

[DPE-3737] Add a zookeeper_client interface #144

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

Conversation

Batalex
Copy link

@Batalex Batalex commented Mar 15, 2024

This PR adds a zookeeper interface.

Notes:

  • The interface is zookeeper instead of zookeeper_client to keep compatibility with existing charms
  • Tests are not implemented. I wrote a recap of stuff that don't work out in the following PR: [DPE-3737] Implement ZK client interface zookeeper-operator#142
    UPDATE: Turns out the tester design is not compatible with the way our charms update the relation databag (=not as a result of "zookeeper-relation-changed")

Copy link
Contributor

@marcoppenheimer marcoppenheimer left a comment

Choose a reason for hiding this comment

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

Left some nitpicks and a couple suggestions! I haven't had a chance to go through with a fine-comb yet, but will do when the PR is open.

Overall it looks good! I haven't seen the Pydantic use for client-relation-interfaces yet, and I'm less familiar with what actually is expected for secret-relations, so I'll defer my opinions to those more in the know.

Oh, and I suggest you just open a branch in the repo, rather than using forks. Makes it easier whenever other people from the team want to make changes to a PR branch!

docs/json_schemas/zookeeper_client/v0/provider.json Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/charms.yaml Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/charms.yaml Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/schema.py Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/schema.py Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/schema.py Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/README.md Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/schema.py Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/schema.py Outdated Show resolved Hide resolved
@Batalex
Copy link
Author

Batalex commented Mar 19, 2024

Thanks for the review!

Oh, and I suggest you just open a branch in the repo, rather than using forks. Makes it easier whenever other people from the team want to make changes to a PR branch!

Unfortunately I don't have permission :(

@Batalex Batalex changed the title WIP: [DPE-3737] Add a zookeeper_client interface [DPE-3737] Add a zookeeper_client interface Mar 20, 2024
interfaces/zookeeper_client/v0/README.md Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/README.md Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/schema.py Outdated Show resolved Hide resolved
interfaces/zookeeper_client/v0/interface_tests/README.md Outdated Show resolved Hide resolved
Still need to remove 'branch' upon upstream merge
@Batalex Batalex force-pushed the feat/DPE-3737 branch 2 times, most recently from 854b72f to c92ca8e Compare September 5, 2024 16:37
@Batalex
Copy link
Author

Batalex commented Sep 6, 2024

Hey there @gruyaume, I updated this PR with the new interface.yaml format. We have red CI due to other charms, should I re-build their json schemas?

interfaces/zookeeper/v0/schema.py Outdated Show resolved Hide resolved
interfaces/zookeeper/v0/schema.py Outdated Show resolved Hide resolved
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.

4 participants