Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement support for IPv6 endpoints #12

Merged
merged 4 commits into from
Mar 27, 2024

Conversation

jedel1043
Copy link
Contributor

@jedel1043 jedel1043 commented Mar 13, 2024

Depends on #10.

Also removes our dependencies on regex. We expect this to not be a problem because we should also be validating the URL from the proxy or the relation interface.

Checklist

  • I am the author of these changes, or I have the rights to submit them.
  • I have added the relevant changes to the README and/or documentation.
  • I have self reviewed my own code.
  • All requested changes and/or review comments have been resolved.

Copy link

@wolsen wolsen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to see 100% unit test coverage of new lines added/modified please.

I think the matrix jobs need to be updated to have ipv6 support as well for the integration tests.

src/utils/manager.py Outdated Show resolved Hide resolved
src/utils/manager.py Outdated Show resolved Hide resolved
src/utils/manager.py Outdated Show resolved Hide resolved
@jedel1043 jedel1043 force-pushed the fix-issue-9 branch 2 times, most recently from d71387e to 55eb708 Compare March 20, 2024 22:09
@NucciTheBoss NucciTheBoss added Priority: Medium This should be addressed by the end of the sprint. Type: Bug Issue reports a bug, or pull request fixes a bug. labels Mar 21, 2024
@NucciTheBoss
Copy link
Collaborator

See my comment here: #10 (comment)

Bumps juju@3.1 to pull a version that has support for IPv6.
Bumps pytest-operator to avoid using a new version of juju with an old version of pytest-operator.
@jedel1043 jedel1043 marked this pull request as ready for review March 26, 2024 19:31
Copy link
Collaborator

@NucciTheBoss NucciTheBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work here! Just a couple of comments/suggestions.

Also, a note. If one of my review comment starts with [Nit], this means that the comment involves a preference of mine. Not a hard and fast rule 😅

Let me know if you have any questions!

src/utils/manager.py Outdated Show resolved Hide resolved
src/utils/manager.py Outdated Show resolved Hide resolved
src/utils/manager.py Outdated Show resolved Hide resolved
src/utils/manager.py Show resolved Hide resolved
tests/unit/test_manager.py Outdated Show resolved Hide resolved
tests/unit/test_manager.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@NucciTheBoss NucciTheBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@NucciTheBoss
Copy link
Collaborator

NucciTheBoss commented Mar 27, 2024

@jedel1043 I'm good to merge this 🚀

Could you just rebase commit cb7ade8 into two separate ones? You can have one for the changes to _translate that's like "refactor: Simplify NFS URL evaluation" and the changes to the unit tests as "chore(lint): Fix formatting of multiline string".

After that rebase I'll go ahead and land your changes 😎

Edit: See your rebase. Thanks! 👍

@jedel1043 jedel1043 force-pushed the fix-issue-9 branch 2 times, most recently from 87bf5ae to 6bf1b97 Compare March 27, 2024 19:13
This needed some refactoring on the current logic to verify that the host address is enclosed by square brackets, but it's nicer than using a big regex to try to validate at the same time IPv4, IPv6 and registered names.
Uses  to mock the filesystem while creating and deleting the autofs files.

Adds more tests to have 100% coverage on the manager module.
The most critical thing here is that we have to modify the settings from the lxd network to enable ipv6 addresses. This should probably be fine since we already modify the default LXD profile.

Modifies CI to run integration tests using both IPv4 and IPv6.
@NucciTheBoss NucciTheBoss merged commit 0f9a009 into canonical:main Mar 27, 2024
8 checks passed
@jedel1043 jedel1043 deleted the fix-issue-9 branch March 27, 2024 19:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: Medium This should be addressed by the end of the sprint. Type: Bug Issue reports a bug, or pull request fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants