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/08/04 11:37:11 UTC
[GitHub] [airflow] coopergillan opened a new pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
coopergillan opened a new pull request #10156:
URL: https://github.com/apache/airflow/pull/10156
`DbApiHook` allows for a `conn_name_attr` to be changed in subclasses, however `SqliteHook`'s `get_conn` method is always calling the main class attribute. Find the correct attribute and use this to establish the connection.
Add pylint override to allow attribute setting outside init for test case.
This PR came out of https://github.com/apache/airflow/pull/10148.
<!--
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] potiuk merged pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #10156:
URL: https://github.com/apache/airflow/pull/10156
----------------------------------------------------------------
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 edited a comment on pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#issuecomment-668571347
Thanks @coopergillan. I see good contributions coming from you :).
One small friendly comment though :).
I have a kind request to not use the [GH-NNNN] prefix in the subject of the PR. While I understand why you are doing it, we chose a little different approach in the community of Airflow.
Maybe it's not perfectly documented (but please take a look at PR/issue template), but one thing we want to have is tohave as much space in the subject of the PR/commit as possible rather than "clutter" it with - fairly artificial issue prefixes - [GH-NNNN]. We used to have JIRA issues this way and we hated it. It was quite a pain when we prepared release notes etc. It's quite a bit of noise especially that we also have PR numbers in commits after merge.
Our approach is "one change per PR" and rather than Iissue# we are using PR# to refer to each change. So rather than referring to particular issue being solved, we talk about particular PR merged (which might solve an issue) and in Release notes we are referring to this PR (not to the issue). It sounds counter-intuitive at first, but we think it's actually better as you can go directly to the code solving the problem by referring to PR rather than add an extra hop. The PR# number is automatically added by Github when you merge PR (with our squash+rebase) method. It adds (#NNN) added so if you refer to an issue in the subject of the commit you have two different numbers there:
```
[G-XXXX] Some description (#YYYY)
```
Where XXXX is the issue number and YYYY is the PR number.
We chose PR# number as the "main" index then. If you do have an issue that you want to address with the PR you simply add
'Closes #NNNN' anywhere in the detailed commit message (not the subject) and GitHub will close the issue automatically when it gets merged. This is what I've manually changed at the merge time in the PR of yours.
One drawback of this solution (but one we accepted as a community) is that you have to split your PR into several PRs if they are referring to different things (particularly if they are solving two issues). It's a bit of a pain especially when you work on a chain of dependent changes (been there, done that) but we think long term it brings much better cherry-pickability and overall quality of the changes.
I hope it explains the reasons why we are doing 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] turbaszek commented on a change in pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#discussion_r464995023
##########
File path: airflow/providers/sqlite/hooks/sqlite.py
##########
@@ -28,12 +28,12 @@ class SqliteHook(DbApiHook):
conn_name_attr = 'sqlite_conn_id'
default_conn_name = 'sqlite_default'
- supports_autocommit = False
Review comment:
It seems this value was hardcoded for `SqliteHook` so I'd say this may be a breaking change
----------------------------------------------------------------
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 #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#issuecomment-668596803
> I'd be happy to lend a hand with updating the contributing docs.
I will CC: you :)
----------------------------------------------------------------
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] coopergillan commented on a change in pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#discussion_r464996172
##########
File path: airflow/providers/sqlite/hooks/sqlite.py
##########
@@ -28,12 +28,12 @@ class SqliteHook(DbApiHook):
conn_name_attr = 'sqlite_conn_id'
default_conn_name = 'sqlite_default'
- supports_autocommit = False
Review comment:
Happy to change it, but since the `DbApiHook` has the same, is it needed? Maybe needed just to be explicit?
----------------------------------------------------------------
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 edited a comment on pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#issuecomment-668571347
Thanks @coopergillan. I see good contributions coming from you :).
One small friendly comment though :).
I have a kind request to not use the [GH-NNNN] prefix in the subject of the PR. While I understand why you are doing it, we chose a little different approach in the community of Airflow.
Maybe it's not perfectly documented (but please take a look at PR/issue template), but one thing we want to have is to have as much space in the subject of the PR/commit as possible rather than "clutter" it with - fairly artificial issue prefixes - [GH-NNNN]. We used to have JIRA issues this way and we hated it. It was quite a pain when we prepared release notes etc. It's quite a bit of noise, especially that we also have PR numbers in commits after merge.
Our approach is "one change per PR" and rather than Iissue# we are using PR# to refer to each change. So rather than referring to a particular issue being solved, we talk about particular PR merged (which might solve an issue) and in Release notes, we are referring to this PR (not to the issue). It sounds counter-intuitive at first, but we think it's actually better as you can go directly to the code solving the problem by referring to PR rather than add an extra hop. The PR# number is automatically added by Github when you merge PR (with our squash+rebase) method. It adds (#NNN) added so if you refer to an issue in the subject of the commit you have two different numbers there:
```
[GH-XXXX] Some description (#YYYY)
```
Where XXXX is the issue number and YYYY is the PR number.
We chose the PR# number as the "main" index then. If you do have an issue that you want to address with the PR you simply add 'Closes #NNNN' anywhere in the detailed commit message (not the subject) and GitHub will close the issue automatically when it gets merged. This is what I've manually changed at the merge time in the PR of yours.
One drawback of this solution (but one we accepted as a community) is that you have to split your PR into several PRs if they are referring to different things (particularly if they are solving two issues). It's a bit of a pain especially when you work on a chain of dependent changes (been there, done that) but we think long term it brings much better cherry-pickability and overall quality of the changes.
I hope it explains the reasons why we are doing 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] coopergillan commented on pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#issuecomment-668578490
@potiuk this is a great explanation and makes perfect sense. I thought it was nice to have the link to the issue auto generated but totally understand how it creates clutter. Much better to take you straight to the pull request.
I didn't know about the auto closing of the issue when referenced in the commit message. Will definitely keep that in mind!
I'd be happy to lend a hand with updating the contributing docs.
Keep up the awesome 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] turbaszek commented on a change in pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#discussion_r464997649
##########
File path: airflow/providers/sqlite/hooks/sqlite.py
##########
@@ -28,12 +28,12 @@ class SqliteHook(DbApiHook):
conn_name_attr = 'sqlite_conn_id'
default_conn_name = 'sqlite_default'
- supports_autocommit = False
Review comment:
Ok, I checked the `DbApiHook`, sounds reasonable.
----------------------------------------------------------------
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 edited a comment on pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#issuecomment-668571347
Thanks @coopergillan. I see good contributions coming from you :).
One small friendly comment though :).
I have a kind request to not use the [GH-NNNN] prefix in the subject of the PR. While I understand why you are doing it, we chose a little different approach in the community of Airflow.
Maybe it's not perfectly documented (but please take a look at PR/issue template), but one thing we want to have is to have as much space in the subject of the PR/commit as possible rather than "clutter" it with - fairly artificial issue prefixes - [GH-NNNN]. We used to have JIRA issues this way and we hated it. It was quite a pain when we prepared release notes etc. It's quite a bit of noise, especially that we also have PR numbers in commits after merge.
Our approach is "one change per PR" and rather than Iissue# we are using PR# to refer to each change. So rather than referring to particular issue being solved, we talk about particular PR merged (which might solve an issue) and in Release notes we are referring to this PR (not to the issue). It sounds counter-intuitive at first, but we think it's actually better as you can go directly to the code solving the problem by referring to PR rather than add an extra hop. The PR# number is automatically added by Github when you merge PR (with our squash+rebase) method. It adds (#NNN) added so if you refer to an issue in the subject of the commit you have two different numbers there:
```
[G-XXXX] Some description (#YYYY)
```
Where XXXX is the issue number and YYYY is the PR number.
We chose the PR# number as the "main" index then. If you do have an issue that you want to address with the PR you simply add 'Closes #NNNN' anywhere in the detailed commit message (not the subject) and GitHub will close the issue automatically when it gets merged. This is what I've manually changed at the merge time in the PR of yours.
One drawback of this solution (but one we accepted as a community) is that you have to split your PR into several PRs if they are referring to different things (particularly if they are solving two issues). It's a bit of a pain especially when you work on a chain of dependent changes (been there, done that) but we think long term it brings much better cherry-pickability and overall quality of the changes.
I hope it explains the reasons why we are doing 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] potiuk commented on pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#issuecomment-668571347
Thanks @coopergillan. I see good contributions coming from you :).
One small friendly comment though :).
I have a kind request to not use the [GH-NNNN] prefix in the subject of the PR. While I understand why you are doing it, we chose a little different approach in the community of Airflow.
Maybe it's not perfectly documented (but please take a look at PR/issue template), but one thing we want to have is tohave as much space in the subject of the PR as possible rather than "clutter" it with - fairly artificial issue prefixes - [GH-NNNN]. We used to have JIRA issues this way and we hated it. It was quite a pain when we prepared release notes etc. It's quite a bit of noise especially that we also have PR numbers in commits after merge.
Our approach is "one change per PR" and rather than Iissue# we are using PR# to refer to each change. So rather than referring to particular issue being solved, we talk about particular PR merged (which might solve an issue) and in Release notes we are referring to this PR (not to the issue). It sounds counter-intuitive at first, but we think it's actually better as you can go directly to the code solving the problem by referring to PR rather than add an extra hop. The PR# number is automatically added by Github when you merge PR (with our squash+rebase) method. It adds (#NNN) added so if you refer to an issue in the subject of the commit you have two different numbers there:
```
[G-XXXX] Some description (#YYYY)
```
Where XXXX is the issue number and YYYY is the PR number.
We chose PR# number as the "main" index then. If you do have an issue that you want to address with the PR you simply add
'Closes #NNNN' anywhere in the detailed commit message (not the subject) and GitHub will close the issue automatically when it gets merged. This is what I've manually changed at the merge time in the PR of yours.
One drawback of this solution (but one we accepted as a community) is that you have to split your PR into several PRs if they are referring to different things (particularly if they are solving two issues). It's a bit of a pain especially when you work on a chain of dependent changes (been there, done that) but we think long term it brings much better cherry-pickability and overall quality of the changes.
I hope it explains the reasons why we are doing 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] coopergillan commented on a change in pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#discussion_r464997661
##########
File path: airflow/providers/sqlite/hooks/sqlite.py
##########
@@ -28,12 +28,12 @@ class SqliteHook(DbApiHook):
conn_name_attr = 'sqlite_conn_id'
default_conn_name = 'sqlite_default'
- supports_autocommit = False
Review comment:
[Here's the code over in `DbApiHook`](https://github.com/apache/airflow/blob/a6db84afe1b2824149f735105665da39c2d7b6db/airflow/hooks/dbapi_hook.py#L54).
----------------------------------------------------------------
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 edited a comment on pull request #10156: [GH-10147] Use conn_name_attr for SqliteHook connection
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #10156:
URL: https://github.com/apache/airflow/pull/10156#issuecomment-668571347
Thanks @coopergillan. I see good contributions coming from you :).
One small friendly comment though :).
I have a kind request to not use the [GH-NNNN] prefix in the subject of the PR. While I understand why you are doing it, we chose a little different approach in the community of Airflow.
Maybe it's not perfectly documented (but please take a look at PR/issue template), but one thing we want to have is to have as much space in the subject of the PR/commit as possible rather than "clutter" it with - fairly artificial issue prefixes - [GH-NNNN]. We used to have JIRA issues this way and we hated it. It was quite a pain when we prepared release notes etc. It's quite a bit of noise, especially that we also have PR numbers in commits after merge.
Our approach is "one change per PR" and rather than Iissue# we are using PR# to refer to each change. So rather than referring to a particular issue being solved, we talk about particular PR merged (which might solve an issue) and in Release notes, we are referring to this PR (not to the issue). It sounds counter-intuitive at first, but we think it's actually better as you can go directly to the code solving the problem by referring to PR rather than add an extra hop. The PR# number is automatically added by Github when you merge PR (with our squash+rebase) method. It adds (#NNN) added so if you refer to an issue in the subject of the commit you have two different numbers there:
```
[GH-XXXX] Some description (#YYYY)
```
Where XXXX is the issue number and YYYY is the PR number. That's quite confusing.
We chose the PR# number as the "main" index then. If you do have an issue that you want to address with the PR you simply add 'Closes #NNNN' anywhere in the detailed commit message (not the subject) and GitHub will close the issue automatically when it gets merged. This is what I've manually changed at the merge time in the PR of yours.
One drawback of this solution (but one we accepted as a community) is that you have to split your PR into several PRs if they are referring to different things (particularly if they are solving two issues). It's a bit of a pain especially when you work on a chain of dependent changes (been there, done that) but we think long term it brings much better cherry-pickability and overall quality of the changes.
I hope it explains the reasons why we are doing 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