Ingress address function #35
Conversation
|
LGTM. I think the name makes sense if we will eventually drop the pivate-address bit. |
|
Should this fall back to returning unit_private_ip() if network-get fails with old versions of Juju? Given that all 'provider' charms need to be updated to support cross model relations, if there is no fallback then these charms will likely drop Juju 1.25 support at the same time. Which is not necessarily a terrible thing. |
charmhelpers/core/hookenv.py
Outdated
| addresses = [] | ||
| for rid in relation_ids(relation_name): | ||
| for unit in related_units(rid): | ||
| addresses.append(ingress_address(rid=rid, unit=unit)) |
There was a problem hiding this comment.
Not a problem here, but something that really does annoy me, is that number of times I see this pattern in code; it's crying out for a helper.
e.g.
RelationAddr = NamedTuple('RelationAddr', 'rid unit')
def iter_units_for_relation_name(relation_name):
for rid in relation_ids(relation_name):
for unit in related_units(rid):
yield RelationAddr(rid, unit)
# usage
addresses = [ingress_address(rid=u.rid, unit=u.unit)
for u in iter_units_for_relation_name(name)]I feel like I need to add this to charmhelpers ...
There was a problem hiding this comment.
I totally agree. I actually think juju should provide a simple:
relation-get --all -r RID
But that is wishful thinking.
I am +1 on your generator.
|
I think the function name maps nicely to what it's asking juju to do. @stub42 makes an interesting point. I realised I don't know how old you need to go with Juju before |
|
Oh, private-address is always there (unless the charm removed it, which is it technically allowed to do). I'm just confusing myself. |
|
Yes just to be clear, this is the other end consuming the relation data that was set with network-get on the other side of the relation. unit-get private-address is now replaced with network-get. @ALL, It sounds like people are OK with the name. Unless Alex wants to hold off for the generator I think this is ready for an -1/+1 vote. |
The new features in Juju set ingress-address and egress-networks on all relations based on network spaces. Ingress-address should be used over private-address when it is available. As this is a new feature we fall back to private-address when ingress-address is not available. This new function consumes the relation data and returns the ingress-address when available or the private-address when not.
Add the iter_units_for_relation_name generator for iterating units based on relation name.
thedac
force-pushed
the
ingress-address
branch
from
1d12e23
to
08016ac
Compare
Ensure that only the swift-proxy units and swift-storage peers have access to direct communication with swift storage daemons. Charm-helpers sync to include ufw module and the ingress_address and iter_units_for_relation_name functions. Please review and merge first: juju/charm-helpers#35 Closes-Bug: #1727463 Change-Id: Id5677edbc40b0b891cbe66867d39d076a94c5436
The new features in Juju set ingress-address and egress-networks on all
relations based on network spaces. Ingress-address should be used
over private-address when it is available. As this is a new feature we
fall back to private-address when ingress-address is not available.
This new function consumes the relation data and returns the
ingress-address when available or the private-address when not.
The code is relatively simple. As a review, I request input on the nomenclature as well. Is ingress_address too vague? Some other options:
get_ingress_address
get_relation_ingress_address
get_remote_ingress_address
etc.