open/close_port supports ICMP protocol; network_get supports Juju 2.0.2

[wallyworld]

Juju open/close_port hook tools now supports ICMP protocol, so we modify the API here to match.

The recently landed network_get enhancement fails with Juju 2.0.2 so that is fixed also.

This code has been tested live by being embedded in the cs:~wallyworld/nrpe-charm

[stub42]

Why are you placing the burden on the call site to handle early versions of Juju 2.0? This method should do what its docstring says, and return the network details for the relation endpoint or raise a NotImplementedError. I don't think we should be returning None here.

[wallyworld]

I wanted to fit in with how the caller had already been written, which was to expect None for older 2.x versions of Juju. The NotImplemented result is when the Juju version is 1.25. The intended semantics as the original author intended were:
Juju 1.25: raise NotImplemented
Juju 2.x (where x<3): return None
Juju 2.3: return some data

The point of this fix was that the recently landed network_get() call was not properly catering for the Juju 2.0.2 case. It only handled 2.1.x or 2.2.x

We need to distinguish between 1.25 and 2.x. In 1.25 it really is NotImplemented since network-get hook tool does not exist at all and we need to fall back to unit-get. For early versions of 2.x, network-get is implemented but the behaviour has changed between early and later 2.x versions.

[stub42]

We don't need to distinguish between Juju versions. We need to distinguish if network_get() does what it claims in the docstring, or not. Which is 'Retrieve the network details for a relation endpoint'. I don't think we can do this with Juju 2.{0-2}, so its NotImplementedError.

If returning 'None' under 2.{0-2} really is the correct thing to do, then the docstring needs to be updated to match the new tri-value return. I doubt that, however. A point of charm-helpers is to abstract these sorts of differences away from charm authors, and having the function return two different versions of 'can't be done' depending on what Juju version is running is exactly the sort of problem that charm-helpers is supposed to smooth away.

[wallyworld]

Perhaps it would be better to raise a different exception, eg AttributeError (with a doc string update)
The problem is that the supported semantics have changed between Juju 2.0.x and later versions.
So if a caller gets AttributeError, they can call network_get_primary_address() instead, which is the required fallback. AttributeError reflects that the caller has invoked network_get() without any args, which is invalid for Juju < 2.1.
The other thing about this is that Juju 2.0.2 is really not supported anymore anyway. So it's almost a moot point.

[stub42]

If network_get_primary_address() is the required fallback, then this helper should be helping and do that for them. Again, this is exactly the sort of compatibility problem that charm-helpers is tasked with smoothing over. The charm-helpers methods should work as best they can, even if it is something like status_set() and application_version_set() logging messages if the tools are not available or principal_unit() falling back to heuristics if the required environment variable is not set, so charm authors do not need to care about what version of Juju their charms are running on (or might be running under in the future). And NotImplemented is used for the case where its just impossible.

There is no reason to distinguish between 'underlying tool does not exist' and 'underlying tool cannot give you the answer you requested'; to the charm author it's the same thing. And maybe they try to handle the case with a fallback if the helper isn't smart enough to do it for them. And maybe that fallback fails too and they need to resort to a second fallback. So in this case:

try:
    preferred_result = network_get(...)
except NotImplementedError:
    try:
        next_preferred_result = network_get_primary(...)
    except NotImplementedError:
        third_preferred_result = unit_private_ip()

The above allows the fallbacks the way you need, without making backwards incompatible changes to the return value. And is in my mind structured in a less confusing way than what this patch is suggesting (two different error handling mechanisms rather than one style):

try:
    preferred_result = network_get(...)
    if preferred_result is None:
        # Under some versions of Juju, network_get() may return None
        secondary_result = network_get_primary_address(...)
except NotImplementedError:
    # Under some versions of Juju, network_get() may raise NotImplementedError
    third_preferred_result = unit_private_ip()

(Of course, if the charm only needed an IP address for a particular relation it should call a charm-helpers method that does exactly that and handles the fallbacks for you. I don't think we have this method yet, but will certainly need one for charms to start using in place of unit_private_ip())

[wallyworld]

I've changed it so that if network_get() is called without the required semantics for the version of Juju on which it is running, or it is just missing from that version of Juju, NotImplementedError is raised in both cases. I think this meets what's needed now.
Yeah, we are missing a charm helper for "network-get --ingress-address"

[stub42]

Looks good, apart from the bare except which can be easily tightened

[stub42]

This should be 'except subprocess.CalledProcessError', so we don't catch exceptions we should almost never catch like out of memory, or unrelated exceptions like open-port is missing from the path.

[wallyworld]

done

[stub42]

+1 , but I don't have merge perms on github