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.

Sign up for GitHub

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

Jump to bottom

add a lib for juju topology #11

Merged

simskij merged 16 commits into main from add-juju-topology

May 9, 2022

Member

simskij commented Apr 14, 2022 •

edited

Issue

This PR adds a common library for JujuTopology. It strips away all of the complexity we've added in other charms in favor of a simple and coherent API.

Solution

Get rid of the abstract base class.
Get rid of the mix between properties and methods.
Add the ability to exclude items from the dict
Add the ability to remap keys in the dict

Context

N/A

Testing Instructions

Try the library with one of our existing charms, replacing the current implementation with the one in the library.
Run all tests, both unit and integration.
Perform manual sanity checks

Release Notes

Adds a reusable library for creating, generating and formatting Juju topology.


          add a lib for juju topology

809388a

simskij force-pushed the add-juju-topology branch from 438d070 to 809388a Compare

April 14, 2022 15:14

simskij requested review from balbirthomas, rbarry82, sed-i and jnsgruk

April 14, 2022 15:21

sed-i reviewed

View reviewed changes

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

rbarry82 reviewed

View reviewed changes

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

tests/test_juju_topology.py Outdated Show resolved Hide resolved

tests/test_juju_topology.py Outdated Show resolved Hide resolved

balbirthomas reviewed

View reviewed changes

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

simskij added a commit that referenced this pull request


          address #11 (comment)

2bb15b1


          address review suggestions

7190f7c

simskij force-pushed the add-juju-topology branch from 2bb15b1 to 7190f7c Compare

April 19, 2022 11:59

simskij added 4 commits

April 19, 2022 14:10


          switch to getters and get rid of can_connect warning

1b4d131


          fix constructor defaults

dbc0924


          remove redundant typing

1d393b5


          Merge branch 'main' into add-juju-topology

9aad9fc

Member

jnsgruk commented Apr 19, 2022

This could also do with a slight adjustment to the top-level README to add a reference to this lib :)

Member Author

simskij commented Apr 19, 2022

This could also do with a slight adjustment to the top-level README to add a reference to this lib :)

Good catch! Thanks!

sed-i previously approved these changes

View reviewed changes

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

tests/test_juju_topology.py Outdated Show resolved Hide resolved

simskij mentioned this pull request

Simplify JujuTopology class interface canonical/prometheus-k8s-operator#240

Closed

simskij mentioned this pull request

Decide on either charm_name or charm and use it consistently #12

Open


          Update lib/charms/observability_libs/v0/juju_topology.py

8ab9c93

Co-authored-by: Leon <82407168+sed-i@users.noreply.github.com>

simskij dismissed sed-i’s stale review via

8ab9c93

April 29, 2022 08:02

simskij requested review from Abuelodelanada and dstathis as code owners

April 29, 2022 08:02

simskij added 5 commits

May 1, 2022 09:34


          update readme

0da1b31


          fix identifier length bug and add doctest

5ea9338


          hardcode expected test output


          remove superfluous whitespaces

b118aea


          Merge branch 'main' into add-juju-topology

dstathis reviewed

View reviewed changes

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved


          rename prefixed dict to label matcher dict

15ec18f

jnsgruk previously approved these changes

View reviewed changes

Member

jnsgruk left a comment

This looks good to me now. Comments are all nits on documentation and nothing concrete, can be punted into another PR if you'd prefer.

README.md Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved


          address majority of review comments

c830aad

simskij dismissed jnsgruk’s stale review via

c830aad

May 9, 2022 09:28


          add type annotations

cda3012

sed-i previously approved these changes

View reviewed changes

lib/charms/observability_libs/v0/juju_topology.py Outdated Show resolved Hide resolved

lib/charms/observability_libs/v0/juju_topology.py Show resolved Hide resolved

simskij dismissed sed-i’s stale review via

9e93708

May 9, 2022 13:36


          Update lib/charms/observability_libs/v0/juju_topology.py

9e93708

Co-authored-by: Leon <82407168+sed-i@users.noreply.github.com>

simskij merged commit dee7b6d into main

simskij deleted the add-juju-topology branch

May 9, 2022 13:36

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

balbirthomas balbirthomas left review comments

dstathis dstathis left review comments

rbarry82 rbarry82 left review comments

jnsgruk jnsgruk left review comments

sed-i sed-i left review comments

Abuelodelanada Awaiting requested review from Abuelodelanada