Skip to content

Support for inheriting public api annotations (#1773)#1805

Merged
eyalkoren merged 11 commits intoelastic:masterfrom
tobiasstadler:fix-1773
Jun 8, 2021
Merged

Support for inheriting public api annotations (#1773)#1805
eyalkoren merged 11 commits intoelastic:masterfrom
tobiasstadler:fix-1773

Conversation

@tobiasstadler
Copy link
Contributor

@tobiasstadler tobiasstadler commented May 7, 2021

What does this PR do?

Fixes #1773

Checklist

  • This is an enhancement of existing features, or a new feature in existing plugins
    • I have updated CHANGELOG.asciidoc
    • I have added tests that prove my fix is effective or that my feature works
    • Added an API method or config option? Document in which version this will be introduced
    • I have made corresponding changes to the documentation
  • This is a bugfix
  • This is a new plugin
    • I have updated CHANGELOG.asciidoc
    • My code follows the style guidelines of this project
    • I have made corresponding changes to the documentation
    • I have added tests that prove my fix is effective or that my feature works
    • New and existing unit tests pass locally with my changes
    • I have updated supported-technologies.asciidoc
    • Added an API method or config option? Document in which version this will be introduced
    • Added an instrumentation plugin? Describe how you made sure that old, non-supported versions are not instrumented by accident.
  • This is something else
@ghost
Copy link

ghost commented May 7, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: Started by user eyalkoren

  • Start Time: 2021-06-08T07:19:40.166+0000

  • Duration: 58 min 0 sec

  • Commit: 445f1a0

Test stats 🧪

Test Results
Failed 0
Passed 2196
Skipped 18
Total 2214

Trends 🧪

Image of Build Times

Image of Tests

💚 Flaky test report

Tests succeeded.

Expand to view the summary

Test stats 🧪

Test Results
Failed 0
Passed 2196
Skipped 18
Total 2214

Copy link
Member

@SylvainJuge SylvainJuge left a comment

Choose a reason for hiding this comment

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

LGTM, approval of PR to enable test on CI

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Looks great, well done! 👏
A few minor suggestions.
For the config option name and description - you would need to verify that the field and method names are properly set, and you will have to run ConfigurationExporterTest so that configuration.asciidoc gets updated.
Thanks a lot, this is a very useful extension to our API!

@tobiasstadler
Copy link
Contributor Author

@eyalkoren Thank you for updating configuration.asciidoc!

Copy link
Contributor

@eyalkoren eyalkoren left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks @tobiasstadler ! ❤️

I just did a few small cosmetic changes in the tests and removed the method matching check on super-annotated-methods being in classes within the configured application packages as it is not required.

@eyalkoren eyalkoren merged commit 6bcc51b into elastic:master Jun 8, 2021
@tobiasstadler
Copy link
Contributor Author

Thank You!

@tobiasstadler tobiasstadler deleted the fix-1773 branch June 8, 2021 08:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

3 participants