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

[Feature/extensions] Add getSettings support for AD #686

Merged

Conversation

dbwiddis
Copy link
Member

Description

Copies the getSettings() method impelmentation from AnomalyDetectorPlugin to AnomalyDetectorExtension. This is the third and final step to complete work already done in SDK #147 and and OS #4519 register AD settings with OpenSearch

Issues Resolved

Fixes SDK #159
Relates to SDK #132

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@owaiskazi19
Copy link
Member

@dbwiddis build is failing

@dbwiddis
Copy link
Member Author

@dbwiddis build is failing

@owaiskazi19 Build is failing because SDK #163 changed the API. You would need to change other code here to comply with that new API.

However, don't bother with that, because 169 and 4705 will change it again. So this is currently blocked by those other PRs.

Suggestion:

  • merge 169 / 4705
  • i will submit another PR here to fix the broken build
  • the tests will then pass for this PR

@dbwiddis
Copy link
Member Author

Updated the RestCreateDetectorAction (which was just a copy of hello world) with the new version of Hello World. Tests should pass once both 4705 and 169 are merged.

@dbwiddis
Copy link
Member Author

169 and 4705 merged, should be able to re-run the tests now.

@owaiskazi19
Copy link
Member

@saratvemulapalli can you take of it? I don't have access to rerun the test for AD.

@saratvemulapalli
Copy link
Member

Kicked them off.
Another hacky way to re-run them is to do a force push without any changes. ;)

@dbwiddis
Copy link
Member Author

Will probably have to do that. I think the rerun is not pulling the updated branches.

@dbwiddis
Copy link
Member Author

Well now it's failing on something else entirely, probably something commented out that's still being tested. I give up.

@dbwiddis
Copy link
Member Author

The BaseNodeRequest class was removed by @nknize in #4702.

@dbwiddis
Copy link
Member Author

dbwiddis commented Oct 11, 2022

Relevant comments:

* @deprecated this class is deprecated and classes will extend TransportRequest directly
 */
// TODO: this class can be removed in main once 7.x is bumped to 7.4.0

@dbwiddis
Copy link
Member Author

All green.

And this, kids, is why you pay attention to those little IDE flags that say you're using deprecated code. :-)

@dbwiddis dbwiddis marked this pull request as draft October 11, 2022 16:27
@dbwiddis dbwiddis marked this pull request as ready for review October 11, 2022 16:44
@dbwiddis
Copy link
Member Author

OK, ready to merge. I'm getting confused by this same fix not working on main here, but I'm not sure it's broken (yet) on main here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants