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

Allow port forwarding from any host IP #171

Merged
merged 1 commit into from
Nov 28, 2023

Conversation

AdrianVollmer
Copy link

See #169. With this commit we are able to define the bind IP address when using port forwarding, which will be passed to --publish of podman-run.


Note: I did not quite understand your logic create_host_port_port_forward(). In the returned PortForward objects, host_port is always random. I believe this to be a bug, so please check my change here if it makes sense. I can split it into a separate PR if you want.

@dcermak
Copy link
Owner

dcermak commented Nov 17, 2023

@AdrianVollmer currently the tests of this are broken, please let me know if you want to fix them or if I shall take it over.

@AdrianVollmer
Copy link
Author

I'll take a look later. The nox tests were fine on my system AFAIC. I'll let you know when I need help

@AdrianVollmer AdrianVollmer force-pushed the allow_bind_ip branch 2 times, most recently from 323b26e to 3a3ee8f Compare November 18, 2023 16:05
@AdrianVollmer
Copy link
Author

Let's try again

Copy link

codecov bot commented Nov 19, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (f713074) 94.64% compared to head (505bc38) 94.50%.

Files Patch % Lines
pytest_container/container.py 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #171      +/-   ##
==========================================
- Coverage   94.64%   94.50%   -0.15%     
==========================================
  Files           9        9              
  Lines        1064     1073       +9     
  Branches      221      224       +3     
==========================================
+ Hits         1007     1014       +7     
- Misses         43       44       +1     
- Partials       14       15       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AdrianVollmer
Copy link
Author

Added the tests. Coverage passes on my system. Third time's the charm :)

Copy link
Owner

@dcermak dcermak left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you very much for your contribution!

Apologies for taking so long to get back, I got sick last week.

I have added two minor nitpicks with suggestions.

pytest_container/inspect.py Show resolved Hide resolved
@@ -12,6 +12,10 @@ Breaking changes:
Improvements and new features:


- Add property :py:attr:`~pytest_container.inspect.PortForwarding.bind_ip`
to support binding to arbitrary IP addresses.

Copy link
Owner

Choose a reason for hiding this comment

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

You fixed this one too:

Suggested change
- Fix :py:attr:`~pytest_container.inspect.PortForwarding.host_port` being
ignored when picking the host port

See dcermak#169. With this commit we are able to define the bind IP address
when using port forwarding, which will be passed to `--publish` of
podman-run. Also, it fixes the `host_port` being ignored when specifying
the host port.
@AdrianVollmer
Copy link
Author

No problem. Thanks for your comments.

Unfortunately I don't know how fix that codecov check. If you have an idea how to write a test for that line, feel free to just edit this PR branch.

@dcermak dcermak enabled auto-merge November 28, 2023 07:06
@dcermak dcermak merged commit cbb13a6 into dcermak:main Nov 28, 2023
31 of 33 checks passed
@dcermak dcermak mentioned this pull request Nov 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants