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.

Sign up for GitHub

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

Jump to bottom

Update Oracle check to use python-oracledb library #13298

Merged
merged 39 commits into from
Dec 7, 2022
Merged

Update Oracle check to use python-oracledb library #13298

merged 39 commits into from
Dec 7, 2022

Conversation

Kyle-Neale
Copy link
Contributor

@Kyle-Neale Kyle-Neale commented Nov 10, 2022 •
edited
Loading

What does this PR do?

This PR replaces the old cx_Oracle module with the newer python-oracledb. Since this only replaces cx_Oracle (Instant Client), we still need to keep the JDBC driver implementation for the customer base that still uses it because the documentation nor the source code for python-oracledb lists parameters that we can use to supplement the one's listed in our documentation when using SSL\TLS.

Additional Notes

https://python-oracledb.readthedocs.io/en/latest/index.html
python-oracledb only uses Python 3 so Python 2 environments are removed
#13300 addresses the failing validations

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached
  • If the PR doesn't need to be tested during QA, please add a qa/skip-qa label.

@Kyle-Neale Kyle-Neale requested review from a team as code owners November 10, 2022 19:37
@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@Kyle-Neale Kyle-Neale changed the title Update cx oracle Update Oracle check to use python-oracledb library Nov 10, 2022
@github-actions
Copy link

Label changelog/Changed was added to this Pull Request, so the next release will bump major version. Please make sure this is a breaking change, or use the changelog/Fixed label.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@github-actions
Copy link

The validations job has failed; please review the Files changed tab for possible suggestions to resolve.

@codecov
Copy link

codecov bot commented Nov 10, 2022 •
edited
Loading

Codecov Report

Merging #13298 (8ba54a1) into master (c5cf459) will increase coverage by 0.00%.
The diff coverage is 84.78%.

Flag Coverage Δ
oracle 90.26% <84.78%> (+0.99%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

hithwen
hithwen reviewed Nov 11, 2022
View reviewed changes
oracle/tests/test_jdbc.py Outdated Show resolved Hide resolved
buraizu
buraizu previously approved these changes Nov 30, 2022
View reviewed changes
Copy link
Contributor

@buraizu buraizu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing those changes, this looks good from a docs perspective

hithwen
hithwen requested changes Dec 1, 2022
View reviewed changes
Copy link
Contributor

@hithwen hithwen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small documentation nits

oracle/README.md Outdated Show resolved Hide resolved
oracle/README.md Outdated Show resolved Hide resolved
hithwen
hithwen previously approved these changes Dec 1, 2022
View reviewed changes
@hithwen hithwen dismissed stale reviews from ofek and jose-manuel-almaza December 1, 2022 14:48

adressed

jose-manuel-almaza
jose-manuel-almaza previously approved these changes Dec 1, 2022
View reviewed changes
Copy link
Contributor

@jose-manuel-almaza jose-manuel-almaza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see lines in oracle.py without test coverage but it has a 85%, and I guess that is ok for now

@hithwen
Copy link
Contributor

hithwen commented Dec 1, 2022

I can see lines in oracle.py without test coverage but it has a 85%, and I guess that is ok for now

This PR actually increases in 2.80% the coverage of oracle.py

yzhan289
yzhan289 reviewed Dec 1, 2022
View reviewed changes
Copy link
Contributor

@yzhan289 yzhan289 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

oracle/datadog_checks/oracle/oracle.py Show resolved Hide resolved
oracle/datadog_checks/oracle/oracle.py Show resolved Hide resolved
oracle/README.md Outdated Show resolved Hide resolved
yzhan289
yzhan289 reviewed Dec 2, 2022
View reviewed changes
Copy link
Contributor

@yzhan289 yzhan289 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just some small nits!

oracle/README.md Outdated Show resolved Hide resolved
oracle/datadog_checks/oracle/oracle.py
@@ -165,44 +149,40 @@ def check(self, _):

@property
def _connection(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a comment and is out of scope for this PR, but I think we should eventually abstract out the connection logic to another class, especially since the connection is conditional on either JDBC or oracle client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I think that was something I was planning on doing given more time but it definitely makes sense to do as a future PR.

oracle/tests/test_config.py Outdated Show resolved Hide resolved
Kyle-Neale and others added 2 commits December 2, 2022 13:54
yzhan289
yzhan289 approved these changes Dec 2, 2022
View reviewed changes
Copy link
Contributor

@yzhan289 yzhan289 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔥

@ofek ofek merged commit ef596c3 into master Dec 7, 2022
@ofek ofek deleted the Update-cx_Oracle branch December 7, 2022 16:43
github-actions bot pushed a commit that referenced this pull request Dec 7, 2022
@ofek @buraizu @hithwen
Update Oracle check to use python-oracledb library (#13298)
0c3a629 
* test

* edit and fix tests

* fixed error description

* style and remove pdb

* validate deps

* remove py2 env

* remove ,

* fixed lone except

* small fix

* tcps

* fix

* fix

* change readme

* style fix

* log line fix

* fix e2e

* fix e2e

* fix dependency mismatch

* Update oracle/README.md

Co-authored-by: Bryce Eadie <[email protected]>

* Update oracle/README.md

Co-authored-by: Bryce Eadie <[email protected]>

* Update oracle/README.md

Co-authored-by: Bryce Eadie <[email protected]>

* Update oracle/README.md

Co-authored-by: Bryce Eadie <[email protected]>

* edit and fix tests

* update docs

* fix

* fix

* fix

* Update oracle/datadog_checks/oracle/oracle.py

Co-authored-by: Julia <[email protected]>

* Update oracle/README.md

Co-authored-by: Julia <[email protected]>

* Update oracle/README.md

Co-authored-by: Julia <[email protected]>

* Update oracle/datadog_checks/oracle/oracle.py

Co-authored-by: Julia <[email protected]>

* update docs

* Update oracle/README.md

Co-authored-by: Julia <[email protected]>

* Update oracle/README.md

Co-authored-by: Julia <[email protected]>

* fix

* add PY2 ConfigurationError

* Update oracle/README.md

Co-authored-by: Andrew Zhang <[email protected]>

* fix

Co-authored-by: Bryce Eadie <[email protected]>
Co-authored-by: Julia <[email protected]>
Co-authored-by: Andrew Zhang <[email protected]> ef596c3
This was referenced Dec 14, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ofek ofek ofek approved these changes

@yzhan289 yzhan289 yzhan289 approved these changes

@hithwen hithwen hithwen left review comments

@buraizu buraizu buraizu left review comments

@jose-manuel-almaza jose-manuel-almaza jose-manuel-almaza left review comments

Assignees
No one assigned
Labels
base_package dependencies documentation integration/oracle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Kyle-Neale @hithwen @ofek @yzhan289 @buraizu @jose-manuel-almaza