Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Oracle] Oracle Hook - automatically set current_schema when defined in Connection #19084

Merged
merged 8 commits into from
Feb 7, 2022

Conversation

mehmax
Copy link
Contributor

@mehmax mehmax commented Oct 19, 2021

Per Default, Oracle Hook was using "scheme" field of the Connection only when generating DSN uri.

This PR adds functionality to Oracle Hook.
-> When a schema is provided in the Connection Parameters (maintained in the UI), then this schema is set as current_schema after connecting to the Database

This allows SQLs to be less static.

Before:
SELECT * FROM PROD_SCHEMA.MY_TABLE
SELECT * FROM TEST_SCHEMA.MY_TABLE

Now:
SELECT * FROM MY_TABLE

closes: #18664
^ Add meaningful description above

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 10, 2021
@github-actions github-actions bot closed this Dec 15, 2021
@uranusjr
Copy link
Member

uranusjr commented Dec 15, 2021

Sorry for missing this. This is close to ready IMO so let’s get it finished.

@uranusjr uranusjr reopened this Dec 15, 2021
@uranusjr uranusjr removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Dec 15, 2021
@uranusjr
Copy link
Member

uranusjr commented Feb 3, 2022

The linter is complaining.

@potiuk
Copy link
Member

potiuk commented Feb 3, 2022

Yeah . I am really just about to release (slightly delayed) provider's release so it would be great to get that one ien @mehmax

@mehmax
Copy link
Contributor Author

mehmax commented Feb 7, 2022

The linter is complaining.

Should be fixed now. Sorry

@mehmax mehmax marked this pull request as ready for review February 7, 2022 18:47
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Feb 7, 2022
@github-actions
Copy link

github-actions bot commented Feb 7, 2022

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 main or amend the last commit of the PR, and push it with --force-with-lease.

@potiuk
Copy link
Member

potiuk commented Feb 7, 2022

Just a docker-compose failure which we will fix shorlty.

@potiuk potiuk merged commit 471e368 into apache:main Feb 7, 2022
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 7, 2022

Awesome work, congrats on your first merged pull request!

@malthe
Copy link
Contributor

malthe commented Feb 14, 2022

@potiuk this seems to have broken something in our system – just letting you know for release purposes while I investigate.

@malthe
Copy link
Contributor

malthe commented Feb 14, 2022

This seems to have been fixed in #21524 but I wonder if this was intentional in the sense that the None vs empty string situation was just resolved by chance.

@potiuk
Copy link
Member

potiuk commented Feb 14, 2022

This seems to have been fixed in #21524 but I wonder if this was intentional in the sense that the None vs empty string situation was just resolved by chance.

This was the reason why I "held" the RC2 version for Oracle provider - and yeah, I believe @mehmax found the same compatibility provlem and fixed it (included in RC3). Any more comments @malthe about your error to verify it was the same issue?

@mehmax
Copy link
Contributor Author

mehmax commented Feb 14, 2022

Hi @malthe,
If you were passing the Service Name in the conn.schema field, the this PR might have had negative impact.

With fix in #21524 I not only changed this from a None to an empty string check, but also added conn.extra["service_name"] / service_name to the check

@malthe
Copy link
Contributor

malthe commented Feb 14, 2022

@mehmax I had a conn.schema which was an empty string so I needed the #21524 behavior there as well. I can confirm that rc3 works for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers okay to merge It's ok to merge this PR as it does not require more tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Oracle] Oracle Hook - make it possible to define a schema in the connection parameters
5 participants