You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/04/29 08:16:47 UTC
[GitHub] [airflow] malthe opened a new pull request #15589: [Oracle] Add port to DSN
malthe opened a new pull request #15589:
URL: https://github.com/apache/airflow/pull/15589
This adds the port (if specified) to the DSN string.
Previously, a port would be ignored.
<!--
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 edited a comment on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835188482
Yeah. Can you please rebase on top of the latest master? There were a couple of problems in master that have been just fixed.
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835186869
These test failures seem unrelated to this PR.
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835729258
@potiuk done!
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-833818061
@potiuk I think the test failure is unrelated. I noticed this in the test log:
```
Yarn requires Node.js 4.0 or higher to be installed.
```
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835768889
@potiuk fixed whitespace – using `black` locally helps :-)
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835811739
Actually having `pre-commit` installed locally helps even more :)
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829049303
Could you please add a unit test for 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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829608618
@potiuk I have cleaned up the docs and code to emphasize that _Host_ and _Schema_ (and the rest of the standard fields) are perfectly good for most users.
If a user has an Oracle DSN (Data Source Name) then they can leave _Host_ empty and use the `dsn` extra.
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829590431
@potiuk just a quick question – the Oracle-specific fields in the connection documentation (e.g. "Sid" and "Service_name"), those don't make sense to me at all looking at: https://github.com/apache/airflow/blob/df143aee8d9e7e0089b747bdd27addf63bb4962f/airflow/www/forms.py#L212
Is this simply out of date?
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-830627494
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-832818739
@potiuk so this still needs two additional reviews is that how the process works?
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829075499
@potiuk done – and I have fixed a minor bug (it shouldn't add the port suffix if it's the default port).
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829428066
> So if we want host:port/<something> to be translated back to host:port/<something>, then it seems conn.schema is the way to go.
Hmm. I think this is not how (at least originally) the oracle connection was designed so far - it never mentions schema in the docs and it makes a clear distinction between SID (seems older system) and Service_name - seems like a newer one.
The docs https://airflow.apache.org/docs/apache-airflow-providers-oracle/stable/connections/oracle.html are slightly misleading I think (They do not mention that service_name is passed as service_name extra.
However I think I am perfectly fine with using schema as service_name in case the latter is not present. That makes sense and makes it consistent with other DBAPIHooks (for example MySQL https://airflow.apache.org/docs/apache-airflow-providers-mysql/stable/connections/mysql.html) . Would you mind also updating the docs for it ?
And yeah. That would indeed be nicer URL in many cases:
`host:port/service`
rather than
`host:port?__extra__service_name=service`
But for backwards compatibility we should support both
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-833833320
[The Workflow run](https://github.com/apache/airflow/actions/runs/818128334) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^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] potiuk commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829428066
> So if we want host:port/<something> to be translated back to host:port/<something>, then it seems conn.schema is the way to go.
Hmm. I think this is not how (at least originally) the oracle connection was designed so far - it never mentions schema in the docs and it makes a clear distinction between SID (seems older system) and Service_name - seems like a new one.
The docs https://airflow.apache.org/docs/apache-airflow-providers-oracle/stable/connections/oracle.html are slightly misleading I think (They do not mention that service_name is passed as service_name extra.
However I think I am perfectly fine with using schema as service_name in case the latter is not present. That makes sense and makes it consistent with other DBAPIHooks (for example MySQL https://airflow.apache.org/docs/apache-airflow-providers-mysql/stable/connections/mysql.html) . Would you mind also updating the docs for 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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829388253
@potiuk yes, but when you're just providing a connection string, `service_name` is not what gets populated, `conn.schema` is.
So if we want `host:port/<something>` to be translated back to `host:port/<something>`, then it seems `conn.schema` is the way to go.
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835188482
Yeah. Can you please rebae on top of the latest master? There were a couple of problems in master that have been just fixed.
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-832820049
No. I t just needs fixes of the failing tests (just make sure to rebase it to latest master and make sure all the checks are green)
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835740658
@potiuk fixed – thanks for the patience!
(I had a missing newline in the rst markup.)
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #15589:
URL: https://github.com/apache/airflow/pull/15589
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829428066
> So if we want host:port/<something> to be translated back to host:port/<something>, then it seems conn.schema is the way to go.
Hmm. I think this is not how (at least originally) the oracle connection was designed so far - it never mentions schema in the docs and it makes a clear distinction between SID (seems older system) and Service_name - seems like a new one.
The docs https://airflow.apache.org/docs/apache-airflow-providers-oracle/stable/connections/oracle.html are slightly misleading I think (They do not mention that service_name is passed as service_name extra.
However I think I am perfectly fine with using schema as service_name in case the latter is not present. That makes sense and makes it consistent with other DBAPIHooks (for example MySQL https://airflow.apache.org/docs/apache-airflow-providers-mysql/stable/connections/mysql.html) . Would you mind also updating the docs for it ?
And yeah. That would indeed be nicer URL in many cases:
`host:port/service`
rather than
`host:port?__extra__service_name=service`
But for backwards compatibility we should support both
--
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 #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
potiuk edited a comment on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829428066
> So if we want host:port/<something> to be translated back to host:port/<something>, then it seems conn.schema is the way to go.
Hmm. I think this is not how (at least originally) the oracle connection was designed so far - it never mentions schema in the docs and it makes a clear distinction between SID (seems older system) and Service_name - seems like a new one.
The docs https://airflow.apache.org/docs/apache-airflow-providers-oracle/stable/connections/oracle.html are slightly misleading I think (They do not mention that service_name is passed as service_name extra.
However I think I am perfectly fine with using schema as service_name in case the latter is not present. That makes sense and makes it consistent with other DBAPIHooks (for example MySQL https://airflow.apache.org/docs/apache-airflow-providers-mysql/stable/connections/mysql.html) . Would you mind also updating the docs for it ?
And yeah. That would indeed be nicer URL in many cases:
`host:port/service`
rather than
`host:port?__extra__service_name=service`
--
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] boring-cyborg[bot] commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-835811843
Awesome work, congrats on your first merged pull request!
--
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] malthe commented on pull request #15589: [Oracle] Add port to DSN
Posted by GitBox <gi...@apache.org>.
malthe commented on pull request #15589:
URL: https://github.com/apache/airflow/pull/15589#issuecomment-829167561
@uranusjr it seems to me that `conn.schema` should be added there as well:
```python
if conn.schema is not None:
dsn += "/" + conn.schema
```
E.g. `localhost:1521/xe`.
That seems to be right based on my experiments in passing in such a connection string.
--
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