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/04 18:50:17 UTC
[GitHub] [airflow] ashb opened a new pull request #12816: Configuration.getsection now copes with sections that are exist in user config
ashb opened a new pull request #12816:
URL: https://github.com/apache/airflow/pull/12816
If you try to run `airflow config list` with an old config you upgraded
from 1.8, it would fail for any sections that have been removed from the
default cofing -- `ldap` for instance.
This would also be a problem if the user makes a typo in a config
section, or is using the airflow config for storing their own
information.
While I was changing this code, I also removed the use of private
methods/variable access in favour of public API
<!--
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] XD-DENG commented on a change in pull request #12816: Configuration.getsection now copes with sections that are exist in user config
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536315784
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
Shall we have one more assert between `test_conf.getsection('non_existing_section')` and an empty OrderedDict?
----------------------------------------------------------------
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 #12816: Configuration.getsection now copes with sections that are exist in user config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536343240
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
Sure, not difficult - let's leave this better than we found 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] XD-DENG commented on a change in pull request #12816: Configuration.getsection now copes with sections that are exist in user config
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536345615
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
I realize my last question/request may not make sense... Battery seems too low after a long day ;-)
----------------------------------------------------------------
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 #12816: Configuration.getsection now copes with sections that are exist in user config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536319897
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
Good idea
----------------------------------------------------------------
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 #12816: Configuration.getsection now copes with sections that only exist in user config
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#issuecomment-739033951
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] XD-DENG edited a comment on pull request #12816: Configuration.getsection now copes with sections that only exist in user config
Posted by GitBox <gi...@apache.org>.
XD-DENG edited a comment on pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#issuecomment-739023107
A "side" observation: either something is wrong with GitHub iOS app, or something is wrong with our CI pipeline setting.
### When I check the "checks" of this PR on phone:
### meanwhile how it looks in browser on laptop
<img width="860" alt="Screenshot 2020-12-04 at 10 10 53 PM" src="https://user-images.githubusercontent.com/11539188/101215146-96939b80-367d-11eb-8c3e-befa434c87e0.png">
----------------------------------------------------------------
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] XD-DENG commented on a change in pull request #12816: Configuration.getsection now copes with sections that are exist in user config
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536336595
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
May be a bit too picky:
but I think it's worthwhile adding one more assert for a section *only* existing in `airflow_defaults`'s sections (and not existing in env var either). In that case, we should expect an empty OrderedDict rather than `None`.
Just to ensure we cover all cases.
----------------------------------------------------------------
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 pull request #12816: Configuration.getsection now copes with sections that only exist in user config
Posted by GitBox <gi...@apache.org>.
ashb commented on pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#issuecomment-739034498
It's there on desktop when you go through to check details too
----------------------------------------------------------------
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 #12816: Configuration.getsection now copes with sections that only exist in user config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536377462
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
Oh you did.
----------------------------------------------------------------
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] XD-DENG commented on pull request #12816: Configuration.getsection now copes with sections that only exist in user config
Posted by GitBox <gi...@apache.org>.
XD-DENG commented on pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#issuecomment-739023107
A "side" observation: either something is wrong with GitHub iOS app, or something is wrong with our CI pipeline setting.
### When I check the "checks" of this PR on phone:
### meanwhile how it looks in browser on laptop
<img width="860" alt="Screenshot 2020-12-04 at 10 10 53 PM" src="https://user-images.githubusercontent.com/11539188/101215146-96939b80-367d-11eb-8c3e-befa434c87e0.png">
----------------------------------------------------------------
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] XD-DENG merged pull request #12816: Configuration.getsection now copes with sections that only exist in user config
Posted by GitBox <gi...@apache.org>.
XD-DENG merged pull request #12816:
URL: https://github.com/apache/airflow/pull/12816
----------------------------------------------------------------
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 #12816: Configuration.getsection now copes with sections that only exist in user config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536377388
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
I think I got what you mean -- check now?
----------------------------------------------------------------
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 #12816: Configuration.getsection now copes with sections that are exist in user config
Posted by GitBox <gi...@apache.org>.
ashb commented on a change in pull request #12816:
URL: https://github.com/apache/airflow/pull/12816#discussion_r536323355
##########
File path: tests/core/test_configuration.py
##########
@@ -375,6 +377,11 @@ def test_getsection(self):
test_conf.getsection('testsection'),
)
+ self.assertEqual(
+ OrderedDict([('key', 'value')]),
+ test_conf.getsection('new_section'),
+ )
+
Review comment:
Returns None it turns out, but yet lets test 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