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

Backport add CreateAlertTool (#349) #456

Merged

Conversation

qianheng-aws
Copy link
Contributor

@qianheng-aws qianheng-aws commented Oct 31, 2024

Description

Original PR: #349

Related Issues

Resolves #[Issue number to be closed when this PR is merged]

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff.
  • Public documentation issue/PR created.

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.

* add CreateAlertTool

Signed-off-by: Heng Qian <qianheng@amazon.com>

* spotlessApply and address comments

Signed-off-by: Heng Qian <qianheng@amazon.com>

* add IT for CreateAlertTool

Signed-off-by: Heng Qian <qianheng@amazon.com>

* address comments

Signed-off-by: Heng Qian <qianheng@amazon.com>

* fix after merging main

Signed-off-by: Heng Qian <qianheng@amazon.com>

* fix forbidden API

Signed-off-by: Heng Qian <qianheng@amazon.com>

* fix IT failure

Signed-off-by: Heng Qian <qianheng@amazon.com>

* run spotlessCheck

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Address comments

Signed-off-by: Heng Qian <qianheng@amazon.com>

* Address comments of changing getIndex to use ActionListener

Signed-off-by: Heng Qian <qianheng@amazon.com>

* fix IT

Signed-off-by: Heng Qian <qianheng@amazon.com>

* make prompt dict static

Signed-off-by: Heng Qian <qianheng@amazon.com>

---------

Signed-off-by: Heng Qian <qianheng@amazon.com>
@qianheng-aws qianheng-aws force-pushed the backport/backport-349-to-2.x branch from 448f6ed to 3b49e97 Compare October 31, 2024 07:45
@qianheng-aws qianheng-aws marked this pull request as ready for review November 1, 2024 07:53
@qianheng-aws qianheng-aws changed the title add CreateAlertTool (#349) Backport add CreateAlertTool (#349) Nov 1, 2024
@yuye-aws
Copy link
Member

yuye-aws commented Nov 1, 2024

@qianheng-aws Can you update the PR description?

@zane-neo
Copy link
Collaborator

zane-neo commented Nov 4, 2024

@qianheng-aws Can you update the PR description?

Right, please add original PR link in the description.

* Fix gradle task forbiddenApisMain failure

Signed-off-by: zane-neo <zaniu@amazon.com>

* fix compilation error since AD change

Signed-off-by: zane-neo <zaniu@amazon.com>

---------

Signed-off-by: zane-neo <zaniu@amazon.com>

(cherry picked from commit 3e63a62)
Signed-off-by: Heng Qian <qianheng@amazon.com>
@@ -275,7 +275,7 @@ public void init(Client client) {
@Override
public CreateAlertTool create(Map<String, Object> params) {
String modelId = (String) params.get(MODEL_ID);
if (Strings.isBlank(modelId)) {
if (org.apache.commons.lang3.StringUtils.isBlank(modelId)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you import the StringUtils?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's cherry-picked from PR #391, We better not change it in this backport PR.

I think if we need to fix it, we better fix in in main branch and backport it to 2.x

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see. Is there any reason why you are directly using org.apache.commons.lang3.StringUtils.isBlank instead of importing it?

@zane-neo zane-neo merged commit de9d588 into opensearch-project:2.x Nov 14, 2024
14 of 16 checks passed
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