canonical / traefik-route-k8s-operator

https://charmhub.io/traefik-route-k8s
Apache License 2.0
0 stars 3 forks source link

v1 api #54

Open PietroPasotti opened 4 months ago

PietroPasotti commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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 commented 4 months ago

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.