You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/02/09 20:20:12 UTC
[GitHub] [airflow] mzaforas opened a new pull request #14157: Option "privileged" in DockerOperator
mzaforas opened a new pull request #14157:
URL: https://github.com/apache/airflow/pull/14157
Support for param "privileged" in DockerOperator. This option is useful when you need to give extended privileges to a container. It's already implemented in docker client method create_host_config() and we only need to offer the option in DockerOperator constructor and pass it to docker client. By default should be False.
<!--
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 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/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+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 [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mdmischief commented on pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
mdmischief commented on pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#issuecomment-788228135
Any guesses on when this will be merged or which release?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mzaforas commented on a change in pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
mzaforas commented on a change in pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#discussion_r585498351
##########
File path: tests/providers/docker/operators/test_docker.py
##########
@@ -260,3 +261,27 @@ def test_extra_hosts(self):
assert 'host_config' in self.client_mock.create_container.call_args[1]
assert 'extra_hosts' in self.client_mock.create_host_config.call_args[1]
assert hosts_obj is self.client_mock.create_host_config.call_args[1]['extra_hosts']
+
+ def test_privileged_true(self):
+ operator = DockerOperator(task_id='test', image='test', privileged=True)
+ operator.execute(None)
+ self.client_mock.create_container.assert_called_once()
+ assert 'host_config' in self.client_mock.create_container.call_args[1]
+ assert 'privileged' in self.client_mock.create_host_config.call_args[1]
+ assert True is self.client_mock.create_host_config.call_args[1]['privileged']
+
+ def test_privileged_false(self):
+ operator = DockerOperator(task_id='test', image='test', privileged=False)
+ operator.execute(None)
+ self.client_mock.create_container.assert_called_once()
+ assert 'host_config' in self.client_mock.create_container.call_args[1]
+ assert 'privileged' in self.client_mock.create_host_config.call_args[1]
+ assert False is self.client_mock.create_host_config.call_args[1]['privileged']
+
+ def test_privileged_missing(self):
+ operator = DockerOperator(task_id='test', image='test')
+ operator.execute(None)
+ self.client_mock.create_container.assert_called_once()
+ assert 'host_config' in self.client_mock.create_container.call_args[1]
+ assert 'privileged' in self.client_mock.create_host_config.call_args[1]
+ assert False is self.client_mock.create_host_config.call_args[1]['privileged']
Review comment:
Ok @dstandish, I have refactored and simplified the tests as you suggest
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#issuecomment-777034640
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mzaforas commented on pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
mzaforas commented on pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#issuecomment-788853363
I have parameterized the tests using mocks. Please give me feedback if more changes are needed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish merged pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
dstandish merged pull request #14157:
URL: https://github.com/apache/airflow/pull/14157
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mzaforas commented on pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
mzaforas commented on pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#issuecomment-777762421
I have fixed the broken test
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] dstandish commented on a change in pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
dstandish commented on a change in pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#discussion_r585027242
##########
File path: tests/providers/docker/operators/test_docker.py
##########
@@ -260,3 +261,27 @@ def test_extra_hosts(self):
assert 'host_config' in self.client_mock.create_container.call_args[1]
assert 'extra_hosts' in self.client_mock.create_host_config.call_args[1]
assert hosts_obj is self.client_mock.create_host_config.call_args[1]['extra_hosts']
+
+ def test_privileged_true(self):
+ operator = DockerOperator(task_id='test', image='test', privileged=True)
+ operator.execute(None)
+ self.client_mock.create_container.assert_called_once()
+ assert 'host_config' in self.client_mock.create_container.call_args[1]
+ assert 'privileged' in self.client_mock.create_host_config.call_args[1]
+ assert True is self.client_mock.create_host_config.call_args[1]['privileged']
+
+ def test_privileged_false(self):
+ operator = DockerOperator(task_id='test', image='test', privileged=False)
+ operator.execute(None)
+ self.client_mock.create_container.assert_called_once()
+ assert 'host_config' in self.client_mock.create_container.call_args[1]
+ assert 'privileged' in self.client_mock.create_host_config.call_args[1]
+ assert False is self.client_mock.create_host_config.call_args[1]['privileged']
+
+ def test_privileged_missing(self):
+ operator = DockerOperator(task_id='test', image='test')
+ operator.execute(None)
+ self.client_mock.create_container.assert_called_once()
+ assert 'host_config' in self.client_mock.create_container.call_args[1]
+ assert 'privileged' in self.client_mock.create_host_config.call_args[1]
+ assert False is self.client_mock.create_host_config.call_args[1]['privileged']
Review comment:
would like to suggest you parameterize this set of tests
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mzaforas commented on pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
mzaforas commented on pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#issuecomment-777027990
Of course @potiuk !
I have added three unit test that cover all possible scenarios with this param.
Let me know if it's ok (this my first contribution to Apache Airflow).
Thanks for your work!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #14157: Option "privileged" in DockerOperator
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #14157:
URL: https://github.com/apache/airflow/pull/14157#issuecomment-777456606
There is a docker test failure :(
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org