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] Implement ZK client interface #142

Merged
merged 28 commits into from
May 28, 2024
Merged

Conversation

Batalex
Copy link
Contributor

@Batalex Batalex commented Apr 25, 2024

The following changes were made to the interface:

  • Replaced* chroot by database
  • Replaced chroot-acl by extra-user-roles
  • Replaced uris by endpoints

*well, not exactly "replaced", we kept the old fields around for backward compatibility.

There are a few issues with the charm-relation-interfaces repository:

  • it needs two repositories to function, local tests can only be run against a remote repo, hence the use of my own fork that should be removed
  • the two repositories need to use the same pydantic version. This is an issue because the interface schema defined in [DPE-3737] Add a zookeeper_client interface charm-relation-interfaces#144 originally used v2, but data-platforms-libs require v1
  • the relation now uses a chain of events to set the databag, which appears to be difficult (impossible?) to test with interface_tester

src/charm.py Outdated Show resolved Hide resolved
src/core/models.py Show resolved Hide resolved
metadata.yaml Outdated Show resolved Hide resolved
@Batalex Batalex changed the title [WIP][DPE-3737] ZK client interface [WIP][DPE-3737] Implement ZK client interface Apr 26, 2024
@Batalex Batalex self-assigned this Apr 26, 2024
@Batalex Batalex changed the title [WIP][DPE-3737] Implement ZK client interface [DPE-3737] Implement ZK client interface Apr 26, 2024
tests/interface/conftest.py Outdated Show resolved Hide resolved
Batalex added a commit to canonical/zookeeper-k8s-operator that referenced this pull request May 2, 2024
Copy link
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.

I'm not sure I have understood why the renaming of the endpoint is needed. I'm a bit afraid that it may creates problems when upgrading (if we have existing relations already)

have we tested that it works fine?

metadata.yaml Outdated Show resolved Hide resolved
src/core/models.py Outdated Show resolved Hide resolved
Copy link
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.

LGTM

Copy link
Contributor

@zmraul zmraul left a comment

Choose a reason for hiding this comment

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

Neat. I liked the deprecation implementation, TIL as well :)

@Batalex Batalex merged commit 5b445f1 into main May 28, 2024
15 checks passed
@Batalex Batalex deleted the feat/DPE-3737-client-interface branch May 28, 2024 07:26
Batalex added a commit to canonical/zookeeper-k8s-operator that referenced this pull request May 28, 2024
* feat: Backport VM charm changes

From canonical/zookeeper-operator#142

* fix: Maybe fix conflicting nodes error?

* fix: Fix attr access in quorum manager

* chore: Fix type lint issues

* feat: Backport changes from VM charm
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.

3 participants