canonical / operator-libs-linux

Linux helper libraries for the operator framework
Apache License 2.0
11 stars 37 forks source link

Snap Get - Error on no value set #87

Open MiaAltieri opened 1 year ago

MiaAltieri commented 1 year ago

Problem

When performing snap get:

        snap_cache = snap.SnapCache()
        mongodb_snap = snap_cache["charmed-mongodb"]
        current_uri = mongodb_snap.get("monitor-uri")

this will cause an error if monitor-uri is not set

Ideal usage

        snap_cache = snap.SnapCache()
        mongodb_snap = snap_cache["charmed-mongodb"]
        if mongodb_snap.get("monitor-uri") == self.monitor_config.uri:
             return 

        # do something

Current usage

        snap_cache = snap.SnapCache()
        mongodb_snap = snap_cache["charmed-mongodb"]
        try:
            if mongodb_snap.get("monitor-uri") == self.monitor_config.uri:
                return
        except snap.SnapError:
            # SnapError occurs when the config option `monitor-uri` is not set.
            pass

        # do something

Desired Behavior

Return None if value is not set

benhoyt commented 1 year ago

Thanks for this. I think you're right it would be a nicer API to return None in the case where a config key isn't present (and it matches Python's dict.get behaviour). However, strictly speaking this wouldn't be a backwards-compatible change, because some callers might be relying on the SnapError to be raised in this case. And I don't think it's worth creating a new major version for a relatively trivial change this like.

One possibility to keep it backwards compatible is to add a default argument that, if provided, would return that value instead of raising (the default would still be to raise SnapError). Something like so:

_raise = object()  # unique object to signal a "raise exception" default

...

    def get(self, key, default=_raise) -> str:
        """Fetch a snap configuration value.

        Args:
            key: the key to retrieve
            default: if specified, return that value if the configuration key is not found
                (the default is to raise SnapError)

        Raises:
            SnapError: if default not provided and the configuration key is not found
        """
        if default is _raise:
            return self._snap("get", [key]).strip()
        else:
            try:
                return self._snap("get", [key]).strip()
            except SnapError:
                return default
MiaAltieri commented 1 year ago

Hi @benhoyt my apologies for missing this message - that implementation sounds perfect thank you!

jnsgruk commented 1 year ago

Ben is out for a couple of weeks - @MiaAltieri are you able to get a PR together and we can get it through review?