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

Finish implementation of connection to multiple cluster with relation aliases #7

Merged
merged 7 commits into from
Jun 15, 2022

Conversation

marceloneppel
Copy link
Member

@marceloneppel marceloneppel commented Jun 9, 2022

Issue

In short this PR address JIRA ticket DPE-423.

In long:

  • Application charms using the requires charm library should be able to connect to multiple database clusters through the same relation.
  • Currently, the library can handle it, but there is no example or test showing how to do that.
  • The custom events names are hardcoded in the library, similar to what is described on Support relation aliases and multiple relations redis-k8s-operator#30.

Solution

  • The items below were previously implemented in Add partial code to handle multiple clusters in the same relation #5:
    • Use the relation id property from the relation linked to the custom events that are triggered inside the library.
    • With the relation id it's possible to know which database cluster the application is talking to at a specific moment and get the relation data specific to that cluster.
  • Add an option to assign an alias to each relation/cluster and use it in the custom events names, so an application charm developer can handle the events in both ways (using just one event observer for multiple clusters or different observers for each cluster).

Context

  • No more code were added to the library related to the change to use the relation id. Everything was functional in the last PR.
  • This PR focused on adding the missing integration test related to the connection to multiple clusters through the same relation.
  • It also focused on adding the option to add an alias to each relation/cluster and use it in the custom events names (so the event names aren't simply hardcoded - there are still the hardcoded events, but now the application charm developer can opt to use different events for different clusters). For that, some code was added to the DatabaseRequires class:
    • relations_aliases attribute that store the relations/clusters aliases and is used to create the custom events names for each relation/cluster.
    • _assign_relation_alias method: used to assign an alias to a relation instance after juju relate is called.
    • _emit_custom_event method: used to emit a custom event for a specific relation/cluster.
    • _get_relation_alias method: used to get the alias of a relation instance.
  • The application tester charm was updated to have an example of how to use the library when it's desired to connect to multiple clusters through the same relation and using aliases for each relation.

Testing

  • Unit tests were updated and some new tests (related to the alias feature) were implemented.
  • One more integration test was implemented to check that an application using the requires side charm library can connect to multiple clusters using the same relation.

Release Notes

  • Finish implementation of connection to multiple clusters using the same relation.

@marceloneppel marceloneppel changed the title Add database clusters aliases Finish implementation of connection to multiple cluster with relation aliases Jun 10, 2022
@marceloneppel marceloneppel marked this pull request as ready for review June 10, 2022 19:00
Copy link
Contributor

@paulomach paulomach left a comment

Choose a reason for hiding this comment

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

I left one blocking (to me) comment. Want to see more comments on that.
All the rest and tests looks good though.

Copy link
Contributor

@WRFitch WRFitch left a comment

Choose a reason for hiding this comment

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

LGTM, a couple of minor nits about style but this looks really solid :)

lib/charms/data_platform_libs/v0/database_requires.py Outdated Show resolved Hide resolved
lib/charms/data_platform_libs/v0/database_requires.py Outdated Show resolved Hide resolved
tests/integration/test_charm.py Outdated Show resolved Hide resolved
@marceloneppel
Copy link
Member Author

@delgod, I implemented a test with the multiple database clusters withouth alias (47995f1).

@marceloneppel marceloneppel merged commit 186ddaf into main Jun 15, 2022
@marceloneppel marceloneppel deleted the DPE-423-multiple-clusters-step-2 branch June 15, 2022 16:30
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