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 2020/12/01 17:31:36 UTC
[GitHub] [airflow] kaxil opened a new pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
kaxil opened a new pull request #12742:
URL: https://github.com/apache/airflow/pull/12742
`[webserver] secret_key` is also a secret liek Fernet key. Allowing
it to be set via _CMD or _SECRET allows uers to use external secret store for it.
<!--
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] mik-laj commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533697937
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
> Here is the issue: https://issues.apache.org/jira/browse/AIRFLOW-5136 that broke 1.10.4 . We started verifying the list of template fields after that.
Comparing the contents of the `template_fields` field would not help us in detecting this problem. You need to check that `template_fields` field contains the fields that are set in the constructor.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533703135
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
How is this supposed to prevent aggression? I think during the review, this value will be added to the two arrays and no one will see the difference until they execute the operator.
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
How is this supposed to prevent reggression? I think during the review, this value will be added to the two arrays and no one will see the difference until they execute the operator.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533696577
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
+ config_parser = AirflowConfigParser()
+ self.assertEqual(
+ config_parser.sensitive_config_values,
Review comment:
```suggestion
sorted(config_parser.sensitive_config_values),
```
This way at least it survives being re-ordered.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533796882
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
:D Maybe a badly chosen example my part or I didn't convey it properly in that case. Let's agree to dis-agree on this one.
OK looks like the tests is failing because of side-effects of the tests above it --- will just remove it
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533699632
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
This is what I meant:
```
assert DataProcSparkOperator.template_fields == ['arguments', 'job_name', 'cluster_name', 'region', 'dataproc_jars', 'dataproc_properties']
```
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533671252
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
I agree with @kaxil, the functionality of `_CMD` and `_SECRET` is already covered in tests above.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533670870
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
For example with jinja_templated strings, it is enough that we can render Jinja templates with different kind of inputs. But we should not check the same things for all operators, just checking that the template_field contains the list we expect it to should be enough. Otherwise we will have tons of tests for each_template_field * Number of Operators
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533686541
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
I think it is good compromise to not test all. But test the behaviour + the keys we expect it to work with.
>You've essentially just re-written the same code twice 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533675147
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
> template_field contains the list we expect it to should be enough
It is not common.
> Otherwise we will have tons of tests for each_template_field * Number of Operators
In my opinion, it is enough to verify that this behavior only works for one key. We don't need to check that this will work for all keys if the key list is a static list. If something works for one value, it will also work for all other values.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533669561
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
If we start adding test for each configuration, we will lots of duplication that checks exactly the same behaviour.
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
If we start adding test for each configuration, we will have lots of duplication that checks exactly the same behaviour.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533632900
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
The tests should check the behavior, not just the content of the attribute. I cannot imagine a situation when this test could help.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533660364
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
Try removing the config from configuration.py and nothing will fail without this test :) Without it no-one would know why _CMD is not working for sql_alchemy_conn . Hope I changed your imagination ;)
This test tests that these fields are allowed to be run via _CMD or _SECRET.
This PR does not add any functionality and uses existing functionality which is already covered by tests, did you check that?
----------------------------------------------------------------
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] turbaszek commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533796183
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
> This is what I meant:
>
> ```python
> assert DataProcSparkOperator.template_fields == ['arguments', 'job_name', 'cluster_name', 'region', 'dataproc_jars', 'dataproc_properties']
> ```
On this one I must agree with Kamil. This test is useless. This test will work even if someone renames `self.job_name` to `self.magic_dataproc_job_name`. The proper test would be like:
```py
assert all(t in op.__dict__ for t in op.template_fileds)
```
the problem if I'm not mistaken arises when there's difference between `template_fields` and operator attributes.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533687844
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
Here is the issue: https://issues.apache.org/jira/browse/AIRFLOW-5136 that broke 1.10.4 . We started verifying the list of template fields after that.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533671859
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
We don't only write tests for what users directly use. We need to test what the expected behaviour of the attribute would be.
> I don't have a strong opinion on this, but I would prefer the tests to check the behavior, not just the value in the class attribute. I would be fine if we weren't to assume that we have other tests that cover behaviors, and this is just a configuration option.
>
> In real life, no user will write the code:
>
> ```python
> config_parser = AirflowConfigParser()
> print(config_parser.sensitive_config_values)
> ```
>
> so this attribute is an internal implementation detail.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533711395
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
In my opinion, we can merge this PR. This test does not bother me.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533672608
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
I'm with @mik-laj on this -- the test is _tightly_ coupled to the exact implementation detail, meaning even the slightest change (other than formatting) to the code means the test needs to be updated.
You've essentially just re-written the same code twice 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533685483
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
Sorry I still disagree. Basically, what you guys are saying we don't add test for this, right?
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533685956
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
1.10.4 broke for exactly that reason where dataproc operators broke because we didn't have test to verify template_field
>It is not common.
----------------------------------------------------------------
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] kaxil merged pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #12742:
URL: https://github.com/apache/airflow/pull/12742
----------------------------------------------------------------
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 #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#issuecomment-736782163
[The Workflow run](https://github.com/apache/airflow/actions/runs/394393801) is cancelling this PR. Building image for the PR has been cancelled
----------------------------------------------------------------
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 #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#issuecomment-736757328
The PR most likely needs to run full matrix of tests because it modifies parts of the core of Airflow. However, committers might decide to merge it quickly and take the risk. If they don't merge it quickly - please rebase it to the latest master at your convenience, 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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533704205
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
Check the Issue I linked which broke 1.10.4
Also remember, there is a cherry-picking process too which is prone to errors
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533796882
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
:D Maybe a badly chosen example my part or I didn't convey it properly in that case. Let's agree to dis-agree on this one.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533664784
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
The list changes the behaviour of how config is read and used. So verifying it is important in my opinion
----------------------------------------------------------------
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 #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#issuecomment-736938507
[The Workflow run](https://github.com/apache/airflow/actions/runs/395025224) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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] ashb commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533680136
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
> In my opinion, it is enough to verify that this behavior only works for one key. We don't need to check that this will work for all keys if the key list is a static list. If something works for one value, it will also work for all other values.
What he said.
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533699632
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
This is what I meant:
```python
assert DataProcSparkOperator.template_fields == ['arguments', 'job_name', 'cluster_name', 'region', 'dataproc_jars', 'dataproc_properties']
```
----------------------------------------------------------------
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] kaxil commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533796882
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
:D Maybe a badly chosen example in that case. Let's agree to dis-agree on this one.
----------------------------------------------------------------
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] mik-laj commented on a change in pull request #12742: Allow using _CMD / _SECRET to set `[webserver] secret_key` config
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #12742:
URL: https://github.com/apache/airflow/pull/12742#discussion_r533668784
##########
File path: tests/core/test_configuration.py
##########
@@ -557,6 +557,23 @@ def test_command_from_env(self):
# the environment variable's echo command
self.assertEqual(test_cmdenv_conf.get('testcmdenv', 'notacommand'), 'OK')
+ def test_sensitive_config_values(self):
Review comment:
I don't have a strong opinion on this, but I would prefer the tests to check the behavior, not just the value in the class attribute. I would be fine if we weren't to assume that we have other tests that cover behaviors, and this is just a configuration option.
In real life, no user will write the code:
````python
config_parser = AirflowConfigParser()
print(config_parser.sensitive_config_values)
````
so this attribute is an internal implementation detail.
----------------------------------------------------------------
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