feat: add Unit.set_ports() for declarative port opening #1005
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In the majority of cases, the protocol is tcp, so make it a little easier and more concise for those cases.
This avoids the impression that the port is already/always opened, rather than just being a port and the instruction is then given via the open/close methods.
Tests still to be added. This does do a transform to Port() objects that are then disassembled again almost immediately, for the case where ints are provided. We could normalise to a pair of (str, int) instead to avoid that. Not normalising at all ended up with a fairly messy couple of loops instead of simple set operations.
This can be flakey, because we're working with sets but then asserting that things happen in a specific order. That should get resolved.
The opened-ports call should always be first, but after that it depends on the order of items returned from set operations, and we don't care which order they happen in, so compare the sorted items.
jameinel
reviewed
Sep 20, 2023
There was a problem hiding this comment.
Overall this looks good to me. There are a couple bits about test coverage and error messaging.
I do also wonder if we are promoting Port more heavily, whether 'open_port' and 'close_port' should have a way to take them. Probably a step too far, and awkward given compatibility and parameterization.
|
BTW, great to see your first PR show up. |
Add typing overloads to ensure that type checking will raise issues.
Good:
* open_port('icmp')
* open_port('tcp', 1)
* open_port('udp', 2)
* open_port(port=3)
Bad:
* open_port()
* open_port('icmp', 1)
* open_port('tcp')
* open_port('udp')
* open_port(4, 'tcp')
Also check that nothing bad has been done at runtime, raising an error (ModelError to be consistent with ops.testing) if one of the invalid specifications is used.
Technically, 4th 😉 but first one in operator with code, yes 🎉, thanks! Feels great to be moving on from onboarding towards providing value 😄 |
benhoyt
reviewed
Sep 20, 2023
Also some minor adjustments per code review comments.
In other repos, there's code like this: Unfortunately, pyright cannot tell that the protocol argument is always 'tcp' in this case, only that the protocol is one of tcp/udp/icmp, and that's too broad to fit the righter overloaded specs, so it complains. This could be handled with changes in the upstream places, but I don't see any simple way to handle it within ops itself. This was a minor improvement and not really needed, plus it's in the two methods that we're wanting to move people away from, so drop this change.
|
Note: will wait for #918 to be merged before moving this out of draft, so that I can get in the open/close port doc changes and align them with set_port. |
|
I've just merged the #1006 open/close_port docs tweak. |
benhoyt
requested changes
Sep 21, 2023
benhoyt
changed the title
jameinel
approved these changes
Sep 25, 2023
benhoyt
reviewed
Sep 25, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adjustments to port management to provide a more declarative style and some additional brevity in charm code.
OpenPorttoPortto better reflect that it's a protocol+port and the state of the port is changed by calling one of the three relevant methods. KeepOpenPortas an alias for backwards compatibility.Unit.set_ports()method that takes an arbitrary number of ports, either ints (implying 'tcp' as the protocol) orPorts, that will open any ports in the provided list that are not already open, and close any ports that are already open but are not in the provided list.Unit.open_port()andUnit.close_port()default totcpfor the protocol, allowing, for example,unit.open_port(port=8000)Fixes #989