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 2019/12/28 15:39:46 UTC
[GitHub] [airflow] nuclearpinguin opened a new pull request #6938:
[AIRFLOW-6382] Extract provide/create session to session module
nuclearpinguin opened a new pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938
Make sure you have checked _all_ steps below.
### Jira
- [ ] My PR addresses the following [Airflow Jira](https://issues.apache.org/jira/browse/AIRFLOW/) issues and references them in the PR title. For example, "\[AIRFLOW-XXX\] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-6382
### Description
- [ ] Here are some details about my PR, including screenshots of any UI changes:
Extracting provide_session and create_session to separate module
reduces number of cyclic imports and make a disctinction between
session and database.
### Tests
- [ ] My PR adds the following unit tests __OR__ does not need testing for this extremely good reason:
### Commits
- [ ] My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Documentation
- [ ] In case of new functionality, my PR adds documentation that describes how to use it.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the [Updating.md](https://github.com/apache/airflow/blob/master/UPDATING.md) so we can assign it to a appropriate 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
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591084955
(excuse the grump, but this change, and the pre commit rule we put in place meant I had to work around it have code live in the repo but work on master and in 1.10.)
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569427448
Regarding backward compatibility - it works only for `provide_session`, should I import `create_session` in `airflow.utils.db` to preserve full backward compatibility?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569427928
> Regarding backward compatibility - it works only for provide_session, should I import create_session in airflow.utils.db to preserve full backward compatibility?
I think so - it could have been used in plugins as well.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6938:
[AIRFLOW-6382] Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#discussion_r361807098
##########
File path: airflow/utils/session.py
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
In my opinion, this is not a problem because it will be in utils package. It often happens that additional utility files are created, if needed. `db` is a generic package. `db_session` is a specific package. LGTM.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569442066
> I restarted the latest build - it was failing with cassandra problem
It's 3 time at this build. It makes me think there's something build / cache / machine dependent? Moreover we have the skip decorator... #6926
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591091208
>And how did this reduce cyclic imports when utils.db imports the new file still?!
That is not a problem. The main reason is that db.py imports all models, and plenty of models methods uses provide_session decorator.
So every model imported db.provide_session. And db.py imported every model.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569441014
I restarted the latest build - it was failing with cassandra problem
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591091572
Yeah, so long that I at first edited your comment instead of reply :D
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569427350
> But I think it needs rebase now after I merged #6936 as it contains some unrelated changes.
>
I rebased before opening a PR because I saw merge conflict ✅
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin merged pull request #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin merged pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk edited a comment on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569427788
@ash @kaxil @mik-laj -> @nuclearpinguin hit those cyclic imports when fixing pylint issues (and I helped a bit to unentangle it). It would be great if you take a look as well I think.
One comment - I think it's great example when you see pylint cyclic imports detection is actually useful.
It shows this kind of places where things are accidentally coupled in the same module but are really from different levels of abstractions/should be separated out.
In this example - there is really little motivation to keep @provide_session and resetdb() (for example) in the same module - especially if you see that provide_session is USED by models and resetdb USES models - that's where the cyclic import came from.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591096260
It's probably fine as it is. Def not worth changing all the files again anyway.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591081255
Why did we make this change?
Neither the PR nor the linked Jira give _any_ justification for the change. It doesn't reduce any imports, and personally, I found the previous name _clearer_.
Now seeing that code for the first time my question would be "what is it session?"
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569427788
@ash @kaxil @mik-laj -> we hit those cyclic imports with @nuclearpinguin when fixing pylint issues. It would be great if you take a look as well I think.
One comment - I think it's great example when you see pylint cyclic imports detection is actually useful.
It shows this kind of places where things are accidentally coupled in the same module but are really from different levels of abstractions/should be separated out.
In this example - there is really little motivation to keep @provide_session and resetdb() (for example) in the same module - especially if you see that provide_session is USED by models and resetdb USES models - that's where the cyclic import came from.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6938:
[AIRFLOW-6382] Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#discussion_r361807098
##########
File path: airflow/utils/session.py
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
In my opinion, this is not a problem because it will be in utils package. It often happens that additional utility files are created, if 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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569430073
> I rebased before opening a PR because I saw merge conflict ✅
> But i see some changes that are unrelated (and seem like reverting change from #6936. Unless it's github interface problem (seen that in the past) it is a bad conflict resolution. See here for example: [7be9e73#diff-c834cf104193b3b493916d646bc5d386R132](https://github.com/apache/airflow/commit/7be9e73b7af788847753933edbb49b28e7b40840#diff-c834cf104193b3b493916d646bc5d386R132)
I rebased one more time and it seems to be ok.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ashb edited a comment on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591081255
> And how did this reduce cyclic imports when utils.db imports the new file still?!
That is not a problem. The main reason is that db.py imports all models, and plenty of models methods uses `provide_session` decorator.
So every model imported db.provide_session. And db.py imported every model.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591092356
That was a big cycle removal. As discussed above, I don't know any other "session" in airflow so the `session.py` was quite intuitive. Do you think it would be worth to rename it to `db_session` ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591091381
:man_facepalming: oh yeah. Sorry, 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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569444636
Shall we :) @nuclearpinguin ?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ashb commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591098934
https://github.com/apache/airflow/pull/7538 will I think solve my issue/brain fart.
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6938:
[AIRFLOW-6382] Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#discussion_r361806699
##########
File path: airflow/utils/session.py
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
What do you think to change the name to `db_session.py`? The very word "session" says too little
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] nuclearpinguin commented on a change in pull request
#6938: [AIRFLOW-6382] Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
nuclearpinguin commented on a change in pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#discussion_r361806943
##########
File path: airflow/utils/session.py
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
I see your point but `db_session` makes me to wonder what is the other not-db-session?
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ashb edited a comment on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591081255
Why did we make this change?
Neither the PR nor the linked Jira give any justification for the change. It doesn't reduce any imports, and personally, I found the previous name clearer.
Now seeing that code for the first time my question would be "what is it session?"
Edit, sorry, not used to old PR template.
> Extracting provide_session and create_session to separate module
reduces number of cyclic imports and make a disctinction between
session and database.
Can you explain to be what the difference between DB and session is? Cos I wouldn't be able to tell you if you asked me. The session is for the db.
And how did this reduce cyclic imports when utils.db imports the new file still?!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on issue #6938: [AIRFLOW-6382] Extract
provide/create session to session module
Posted by GitBox <gi...@apache.org>.
potiuk commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569428165
> I rebased before opening a PR because I saw merge conflict ✅
But i see some changes that are unrelated (and seem like reverting change from #6936. Unless it's github interface problem (seen that in the past) it is a bad conflict resolution. See here for example: https://github.com/apache/airflow/pull/6938/commits/7be9e73b7af788847753933edbb49b28e7b40840#diff-c834cf104193b3b493916d646bc5d386R132
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] mik-laj commented on a change in pull request #6938:
[AIRFLOW-6382] Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#discussion_r361806699
##########
File path: airflow/utils/session.py
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
What do you think to change the name to `db_session.py`? The word "session" says too little
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] codecov-io commented on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-569444282
# [Codecov](https://codecov.io/gh/apache/airflow/pull/6938?src=pr&el=h1) Report
> Merging [#6938](https://codecov.io/gh/apache/airflow/pull/6938?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/4fd3b899dce0397b045c7311a75d50341c7ccf65?src=pr&el=desc) will **decrease** coverage by `<.01%`.
> The diff coverage is `91.5%`.
[](https://codecov.io/gh/apache/airflow/pull/6938?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #6938 +/- ##
=========================================
- Coverage 84.71% 84.7% -0.01%
=========================================
Files 679 680 +1
Lines 38539 38646 +107
=========================================
+ Hits 32647 32735 +88
- Misses 5892 5911 +19
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/6938?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/cli/commands/rotate\_fernet\_key\_command.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9jbGkvY29tbWFuZHMvcm90YXRlX2Zlcm5ldF9rZXlfY29tbWFuZC5weQ==) | `0% <0%> (ø)` | :arrow_up: |
| [airflow/contrib/auth/backends/google\_auth.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2F1dGgvYmFja2VuZHMvZ29vZ2xlX2F1dGgucHk=) | `0% <0%> (ø)` | :arrow_up: |
| [airflow/contrib/auth/backends/ldap\_auth.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2F1dGgvYmFja2VuZHMvbGRhcF9hdXRoLnB5) | `0% <0%> (ø)` | :arrow_up: |
| [...ow/contrib/auth/backends/github\_enterprise\_auth.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2F1dGgvYmFja2VuZHMvZ2l0aHViX2VudGVycHJpc2VfYXV0aC5weQ==) | `0% <0%> (ø)` | :arrow_up: |
| [airflow/contrib/auth/backends/kerberos\_auth.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2F1dGgvYmFja2VuZHMva2VyYmVyb3NfYXV0aC5weQ==) | `0% <0%> (ø)` | :arrow_up: |
| [airflow/contrib/auth/backends/password\_auth.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9jb250cmliL2F1dGgvYmFja2VuZHMvcGFzc3dvcmRfYXV0aC5weQ==) | `0% <0%> (ø)` | :arrow_up: |
| [airflow/jobs/base\_job.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL2Jhc2Vfam9iLnB5) | `92.19% <100%> (ø)` | :arrow_up: |
| [airflow/models/variable.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvdmFyaWFibGUucHk=) | `93.42% <100%> (ø)` | :arrow_up: |
| [airflow/ti\_deps/deps/prev\_dagrun\_dep.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy90aV9kZXBzL2RlcHMvcHJldl9kYWdydW5fZGVwLnB5) | `80.55% <100%> (ø)` | :arrow_up: |
| [airflow/models/skipmixin.py](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvc2tpcG1peGluLnB5) | `97.56% <100%> (ø)` | :arrow_up: |
| ... and [52 more](https://codecov.io/gh/apache/airflow/pull/6938/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/6938?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/6938?src=pr&el=footer). Last update [4fd3b89...a2c95d0](https://codecov.io/gh/apache/airflow/pull/6938?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] ashb edited a comment on issue #6938: [AIRFLOW-6382]
Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
ashb edited a comment on issue #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#issuecomment-591081255
Why did we make this change?
Neither the PR nor the linked Jira give _any_ justification for the change. It doesn't reduce any imports, and personally, I found the previous name _clearer_.
Now seeing that code for the first time my question would be "what is it session?"
Edit, sorry, not used to old PR template.
> Extracting provide_session and create_session to separate module
> reduces number of cyclic imports and make a disctinction between
> session and database.
Can you explain to be what the difference between DB and session is? Cos I wouldn't be able to tell you if you asked me. The session _is for the db_.
And how did this reduce cyclic imports when utils.db imports the new file still?!
----------------------------------------------------------------
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
With regards,
Apache Git Services
[GitHub] [airflow] potiuk commented on a change in pull request #6938:
[AIRFLOW-6382] Extract provide/create session to session module
Posted by GitBox <gi...@apache.org>.
potiuk commented on a change in pull request #6938: [AIRFLOW-6382] Extract provide/create session to session module
URL: https://github.com/apache/airflow/pull/6938#discussion_r361807840
##########
File path: airflow/utils/session.py
##########
@@ -0,0 +1,63 @@
+# Licensed to the Apache Software Foundation (ASF) under one
Review comment:
I like session. We already have 'provide_session' decorator rather than 'provide_db_session' and we are fine with 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
With regards,
Apache Git Services