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

v1 api #54

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

v1 api #54

wants to merge 3 commits into from

Conversation

PietroPasotti
Copy link
Contributor

Issue

v0's Requirer object had a really bad API:

    def __init__(self, charm: CharmBase, relation: Relation, relation_name: str = "traefik-route"):

this is bad because we can derive relation from relation_name and viceversa.
Modern libraries all take an endpoint_name and use it to fetch the relation(s).

V1 breaks this API by exposing instead:

    def __init__(self, charm: CharmBase, endpoint_name: str = "traefik-route"):

This is not an interface change (the interface version remains the same). But it is a breaking api change, hence the major vbump for the charm lib.

@sed-i
Copy link
Contributor

sed-i commented Jun 20, 2024

V1 breaks this API by exposing instead:

def __init__(self, charm: CharmBase, endpoint_name: str = "traefik-route"):

If that's the only change then perhaps we shouldn't wait a bit longer?

@PietroPasotti
Copy link
Contributor Author

V1 breaks this API by exposing instead:

def __init__(self, charm: CharmBase, endpoint_name: str = "traefik-route"):

If that's the only change then perhaps we shouldn't wait a bit longer?

I don't mind, but I don't think there's many planned changes for this lib, so... what should we be waiting for?

@Abuelodelanada
Copy link

Although the v1 is easier to use because we ask for two arguments (charm and endpoint_name) instead of three (charm, relation and relation_name), now TraefikRouteRequirer class has one more responsibility: getting the relation object.

image

Since we are breaking back-guards compatibility, shouldn't we explore a deeper separation of concerns with a Factory class/function?

Something like this:

class TraefikRouterFactory():

    @staticmethod
    def get_requirer(charm: CharmBase, endpoint_name: str = "traefik-route")
        events = TraefikRouteRequirerEvents()
        stored = StoredState()
        relation = charm.model.get_relation(endpoint_name)
        return TraefikRouteRequirer(charm, events, stored, relation)

This way we inject all the dependencies TraefikRouteRequirer needs, and those dependencies are instantiated/obtained by the Factory class.
We can even go one step forward and generalise the Factory class so it can return a TraefikRouteRequirer or a TraefikRouteProvider.

@PietroPasotti
Copy link
Contributor Author

Although the v1 is easier to use because we ask for two arguments (charm and endpoint_name) instead of three (charm, relation and relation_name), now TraefikRouteRequirer class has one more responsibility: getting the relation object.

image

Since we are breaking back-guards compatibility, shouldn't we explore a deeper separation of concerns with a Factory class/function?

Something like this:

class TraefikRouterFactory():

    @staticmethod
    def get_requirer(charm: CharmBase, endpoint_name: str = "traefik-route")
        events = TraefikRouteRequirerEvents()
        stored = StoredState()
        relation = charm.model.get_relation(endpoint_name)
        return TraefikRouteRequirer(charm, events, stored, relation)

This way we inject all the dependencies TraefikRouteRequirer needs, and those dependencies are instantiated/obtained by the Factory class. We can even go one step forward and generalise the Factory class so it can return a TraefikRouteRequirer or a TraefikRouteProvider.

It being an Object already, I think it's fine if it does 'charmy' things.
Besides this is how all other endpoint requirer/provider implementations do it at the moment, so for consistency, I think it's better to keep it that way.

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.

5 participants