You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Bowrna (via GitHub)" <gi...@apache.org> on 2023/05/15 06:33:50 UTC
[GitHub] [airflow] Bowrna opened a new pull request, #30730: adding docker port expose capability
Bowrna opened a new pull request, #30730:
URL: https://github.com/apache/airflow/pull/30730
closes: #27290
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of an existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in a newsfragment file, named `{pr_number}.significant.rst` or `{issue_number}.significant.rst`, in [newsfragments](https://github.com/apache/airflow/tree/main/newsfragments).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #30730:
URL: https://github.com/apache/airflow/pull/30730#issuecomment-1543567027
can expect delay at my side ( for a week) as i am relocating home.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on a diff in pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on code in PR #30730:
URL: https://github.com/apache/airflow/pull/30730#discussion_r1186127966
##########
airflow/providers/docker/operators/docker.py:
##########
@@ -291,6 +296,9 @@ def __init__(
if skip_on_exit_code
else []
)
+ self.port_bindings = port_bindings or {}
Review Comment:
yes sure. i will edit this part @uranusjr I missed to verify this part before committing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #30730: adding docker port expose capability
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk merged PR #30730:
URL: https://github.com/apache/airflow/pull/30730
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on pull request #30730: adding docker port expose capability
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on PR #30730:
URL: https://github.com/apache/airflow/pull/30730#issuecomment-1537522628
Need tests, it seems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #30730: adding docker port expose capability
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #30730:
URL: https://github.com/apache/airflow/pull/30730#discussion_r1184138291
##########
airflow/providers/docker/operators/docker.py:
##########
@@ -291,6 +296,9 @@ def __init__(
if skip_on_exit_code
else []
)
+ self.port_bindings = port_bindings or {}
Review Comment:
Is it really useful to coerce this to dict here? From what I can tell down below, we’re actually converting the value back to None anyway. So it may actually be easier to do `self.port_bindings = port_bindings or None` instead?
##########
airflow/providers/docker/operators/docker.py:
##########
@@ -357,6 +365,7 @@ def _run_image_with_mounts(self, target_mounts, add_tmp_variable: bool) -> list[
command=self.format_command(self.command),
name=self.container_name,
environment={**env_file_vars, **self.environment, **self._private_environment},
+ ports=list(self.port_bindings.keys()) if self.port_bindings else None,
Review Comment:
```suggestion
ports=list(self.port_bindings) if self.port_bindings else None,
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #30730:
URL: https://github.com/apache/airflow/pull/30730#issuecomment-1537929192
yes @uranusjr i will add tests and update here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #30730:
URL: https://github.com/apache/airflow/pull/30730#issuecomment-1532384548
I will add new test for this changes and fix the existing issues in test that breaks because of the changes i have done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] uranusjr commented on a diff in pull request #30730: adding docker port expose capability
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #30730:
URL: https://github.com/apache/airflow/pull/30730#discussion_r1184139118
##########
airflow/providers/docker/operators/docker.py:
##########
@@ -291,6 +296,9 @@ def __init__(
if skip_on_exit_code
else []
)
+ self.port_bindings = port_bindings or {}
Review Comment:
Alternatively, maybe an empty list/dict actually works for Docker’s API? It’d be awesome if you could check; if it does the code could be much simpler.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #30730:
URL: https://github.com/apache/airflow/pull/30730#issuecomment-1546886622
@uranusjr I have added tests to this PR. please check when you have time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on PR #30730:
URL: https://github.com/apache/airflow/pull/30730#issuecomment-1550013644
@uranusjr please take a look when you get time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna closed pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna closed pull request #30730: adding docker port expose capability
URL: https://github.com/apache/airflow/pull/30730
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] Bowrna commented on a diff in pull request #30730: adding docker port expose capability
Posted by "Bowrna (via GitHub)" <gi...@apache.org>.
Bowrna commented on code in PR #30730:
URL: https://github.com/apache/airflow/pull/30730#discussion_r1186608650
##########
airflow/providers/docker/operators/docker.py:
##########
@@ -291,6 +296,9 @@ def __init__(
if skip_on_exit_code
else []
)
+ self.port_bindings = port_bindings or {}
Review Comment:
yes sending an empty list/dict will work. @uranusjr
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org