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 Module] Handle special characters for Oracle connection string properly #31368

Merged
merged 50 commits into from
Jun 17, 2022

Conversation

yug-rajani
Copy link
Contributor

@yug-rajani yug-rajani commented Apr 20, 2022

  • Enhancement

What does this PR do?

This PR enhances the Oracle Metricbeat Module to handle the special characters in the connection string properly.

Why is it important?

Many of the passwords are machine generated and often contain % at the very least, the workaround in the existing module was to not use these characters.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
    - [ ] I have made corresponding change to the default configuration files
  • I have added tests that prove my fix is effective or that my feature works
  • I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 20, 2022
@elasticmachine
Copy link
Collaborator

elasticmachine commented Apr 20, 2022

💚 Flaky test report

Tests succeeded.

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • /package : Generate the packages and run the E2E tests.

  • /beats-tester : Run the installation tests with beats-tester.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@yug-rajani yug-rajani force-pushed the oracle_special_characters branch from 788910b to 7e45828 Compare April 21, 2022 10:26
@mergify
Copy link
Contributor

mergify bot commented Apr 22, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b oracle_special_characters upstream/oracle_special_characters
git merge upstream/main
git push upstream oracle_special_characters

@yug-rajani
Copy link
Contributor Author

/test

@kush-elastic
Copy link
Collaborator

/test

@yug-rajani yug-rajani requested a review from jsoriano May 5, 2022 07:21
@yug-rajani
Copy link
Contributor Author

/test

@yug-rajani yug-rajani added the Team:Service-Integrations Label for the Service Integrations team label May 5, 2022
@botelastic botelastic bot removed the needs_team Indicates that the issue/PR needs a Team:* label label May 5, 2022
@yug-rajani yug-rajani marked this pull request as ready for review May 5, 2022 19:05
@yug-rajani yug-rajani requested a review from a team as a code owner May 5, 2022 19:05
@yug-rajani
Copy link
Contributor Author

/test

Copy link
Contributor

@agithomas agithomas left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@mdelapenya mdelapenya left a comment

Choose a reason for hiding this comment

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

@yug-elastic I'm adding a 👍 in terms of the e2e tests as, for this PR, they have suffered a glitch in the parallel stages that they run. Because the vast majority of the tests are passing, and after a deeper investigation on the existing failures, I'd say that they are temporary and not related to this PR

@yug-rajani
Copy link
Contributor Author

/test

@yug-rajani
Copy link
Contributor Author

@yug-elastic I'm adding a 👍 in terms of the e2e tests as, for this PR, they have suffered a glitch in the parallel stages that they run. Because the vast majority of the tests are passing, and after a deeper investigation on the existing failures, I'd say that they are temporary and not related to this PR

Thank you for the approval, @mdelapenya! I wonder if it would be possible to merge this PR until we have a green checkmark over beats-ci/pr-merge check. If it's possible, kindly help us with it as we don't have the authority to merge this PR.

FYI, we have an approval from @agithomas who is the code owner of this change.

@yug-rajani
Copy link
Contributor Author

/test

@yug-rajani
Copy link
Contributor Author

/test

1 similar comment
@yug-rajani
Copy link
Contributor Author

/test

@yug-rajani
Copy link
Contributor Author

/test

@mergify
Copy link
Contributor

mergify bot commented Jun 15, 2022

This pull request is now in conflicts. Could you fix it? 🙏
To fixup this pull request, you can check out it locally. See documentation: https://help.github.com/articles/checking-out-pull-requests-locally/

git fetch upstream
git checkout -b oracle_special_characters upstream/oracle_special_characters
git merge upstream/main
git push upstream oracle_special_characters

@cmacknz
Copy link
Member

cmacknz commented Jun 17, 2022

The E2E tests here are being triggered because of the updates to go.mod. There's nothing there that looks like it should be causing test failures, and I've seen the same set of tests failing on other branches recently, like https://beats-ci.elastic.co/job/e2e-tests/job/e2e-testing-mbp/job/main/1256/ for one example.

I'm going to merge this.

@cmacknz cmacknz merged commit 1461bad into elastic:main Jun 17, 2022
@yug-rajani
Copy link
Contributor Author

Thanks a lot, @cmacknz!

chrisberkhout pushed a commit that referenced this pull request Jun 1, 2023
…g properly (#31368)

* Handle special characters for Oracle connection string properly
* Resolve some of the lint issues
* Format errors correctly as per lint
* Run make update to resolve pipeline issues
* Update existing test cases to avoid code duplication linting issue
* Add CHANGELOG entry
* Update approach to discard custom parser and use DSN
* Update implementation and tests
* Update handling for service.address host
* Resolve some of the lint issues
* Resolve lint issues
* Update handling for service.address connection string
* Update handling for extraction of host
* Update as per new host parsing mechanism for Oracle
* Resolve CI linting issues
* Update documentation as per review comments
* Update documentation as per review comment
* Run make update
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Service-Integrations Label for the Service Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Metricbeat : SQL & Oracle Modules] Connection string for Oracle does not handle special characters properly
7 participants