Make inability to determine Openstack codename version non-fatal

[canonical-xavd]

This change allows error_out callers to specify a function signalling an error, appropriate to it's context.

Related change: https://review.opendev.org/c/openstack/charm-keystone/+/835087
Related issue: LP#1965966

[ajkavanagh]

This seems like a lot of work to go to? couldn't you just do:

raise ValueError(str(e))

instead if you wanted a change in behaviour here?

[canonical-xavd]

I think it stemmed from an abundance of caution in context of the specific case I'm proposing this change for to not break other charms, but now that it has spilled over to the common codebase, this is indeed more sensible to do.

[ajkavanagh]

I'm currently pushing back on this, as the name of the function error_out is pretty unambiguous that it's going to quit. I think this change blurs the meaning using an optional param which isn't very explicit.

[ajkavanagh]

Sorry, it's been a while since I got back to this. This is a still a change in behaviour. Previously, this function killed the charm hook dead (using error_out()). The proposal is to change it to a ValueError. Now all the charms that assumed that it would die at that point, now get an exception that they haven't got code to handle.

What are you trying to achieve please? Do you have some sample code that uses this function and wants to 'handle' this exception when get_os_codename_version() is called?

The usual way of handling this is to do something like this:

def get_os_version_codename(
        codename,
        version_map=OPENSTACK_CODENAMES,
        raise_exception=False):
    '''Determine OpenStack version number from codename.'''
    for k, v in version_map.items():
        if v == codename:
             return k
    e = 'Could not derive OpenStack version for '\
        'codename: %s' % codename
    if raise_exception:
        raise ValueError(str(e))
    error_out(e)


# use it, but with an exception
try:
    codename = get_os_version_codename(codename, raise_exception=True)
except ValueError:
    # do something with the fact no version maps to the codename

This way the old behaviour is preserved (for charms expecting it), and the new behaviour can be used by the charms wanting the exception rather than exiting the hook.

[canonical-xavd]

The contribution this stems from aims to handle Openstack release codename inference, when it's not possible to deduce it from apt repository specification while using a custom repository, per linked related change to the Keystone charm and linked Launchpad issue, which also serve as code samples demonstrating intent.

Based on the previous proposed change, your current proposition gets half of the way there, as error_out would be parameterized to no longer exclusively fatally sys.exit(), but not allow for costumization of this behavior where it may be contextually appropriate, while preserving currently expected behavior (per lambda provided as default function argument value in the original changeset revision).

[ajkavanagh]

I'm obviously not explaining my point well. The problem with the code in the linked example is this:

def get_os_version_codename(codename, version_map=OPENSTACK_CODENAMES):
    '''Determine OpenStack version number from codename.'''
    for k, v in six.iteritems(version_map):
        if v == codename:
            return k
    e = 'Could not derive OpenStack version for '\
        'codename: %s' % codename
    error_out(e, raiser=ValueError)

The last part changes the behaviour of get_os_version_codename() which other charms rely on. This means that get_os_version_codename() now raises a ValueError, and doesn't do sys.exit() because error_out() is also changed. These are identical in the get_os_version_codename() function as changed above.

error_out(e, raiser=ValueError)
# or
raise ValueError(str(e))

Which makes it worse as it now redefines what error_out does, which is no longer "erroring out of the hook", but "raise a ValueError instead, if told". Semantically, these things are different and will lead to confusion in the future. It matters what things are named and that this matches what the thing does.

That's the problem. Please don't change the default behaviour of get_os_version_codename().

[ajkavanagh]

LGTM - thanks for the updates.