[Enhancement] Enhance validation for create connector API #3260
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
akolarkunnu
requested review from
b4sjoo,
dhrubo-os,
jngz-es,
model-collapse,
rbhavna,
ylwu-amzn,
zane-neo,
Zhangxunmt,
austintlee,
HenryL27 and
xinyual
as code owners
akolarkunnu
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
akolarkunnu
requested a deployment
to
ml-commons-cicd-env-require-approval
GitHub Actions
Waiting
akolarkunnu
commented
Dec 6, 2024
ml-algorithms/src/main/java/org/opensearch/ml/engine/algorithms/remote/ConnectorUtils.java
Outdated
Show resolved
Hide resolved
pyek-bot
reviewed
Dec 6, 2024
common/src/test/java/org/opensearch/ml/common/connector/ConnectorActionTest.java
Outdated
Show resolved
Hide resolved
common/src/test/java/org/opensearch/ml/common/connector/ConnectorActionTest.java
Outdated
Show resolved
Hide resolved
|
The overall change looks fine to me, added some nitpicks. Thanks for the contribution! |
akolarkunnu
force-pushed
the
connectorcreatevalidations
branch
from
db98d71 to
f1d1af1
Compare
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
dhrubo-os
reviewed
Dec 9, 2024
common/src/main/java/org/opensearch/ml/common/transport/connector/MLCreateConnectorInput.java
Outdated
Show resolved
Hide resolved
dhrubo-os
reviewed
Dec 9, 2024
common/src/test/java/org/opensearch/ml/common/connector/ConnectorActionTest.java
Outdated
Show resolved
Hide resolved
dhrubo-os
reviewed
Dec 9, 2024
common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java
Outdated
Show resolved
Hide resolved
dhrubo-os
reviewed
Dec 9, 2024
common/src/main/java/org/opensearch/ml/common/connector/ConnectorAction.java
Outdated
Show resolved
Hide resolved
dhrubo-os
reviewed
Dec 9, 2024
common/src/test/java/org/opensearch/ml/common/connector/ConnectorActionTest.java
Outdated
Show resolved
Hide resolved
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
akolarkunnu
force-pushed
the
connectorcreatevalidations
branch
from
f1d1af1 to
22eb03d
Compare
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
This PR addresses the first part of this enhancement "Validate if connector payload has all the required fields. If not provided, throw the illegal argument exception". Validation of fields description, parameters, credential, and request_body are missing. That validations are added in this fix. Added new test cases correspong to these validations and fixed all failing test cases because of these new validations. Partially Resolves opensearch-project#1382 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]>
akolarkunnu
force-pushed
the
connectorcreatevalidations
branch
from
22eb03d to
ff5392b
Compare
akolarkunnu
had a problem deploying
to
ml-commons-cicd-env-require-approval
GitHub Actions
Failure
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
akolarkunnu
temporarily deployed
to
ml-commons-cicd-env-require-approval
GitHub Actions
Inactive
dhrubo-os
approved these changes
Dec 17, 2024
|
LGTM, thanks for adding the null or empty checks for credentials! Just a following up thoughts, if we can check if the credentials is valid in the create API, that would be even better as a further enhancement coming next. Then we can help user ensure the credentials work before calling the model. |
mingshl
approved these changes
Dec 17, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Dec 17, 2024
This PR addresses the first part of this enhancement "Validate if connector payload has all the required fields. If not provided, throw the illegal argument exception". Validation of fields description, parameters, credential, and request_body are missing. That validations are added in this fix. Added new test cases correspong to these validations and fixed all failing test cases because of these new validations. Partially Resolves #1382 Signed-off-by: Abdul Muneer Kolarkunnu <[email protected]> (cherry picked from commit 58903ba)
Ok, copied this to bug #2993 to include in the upcoming enhancement. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR addresses the first part of this enhancement "Validate if connector payload has all the required fields. If not provided, throw the illegal argument exception". Validation of field credentials is added in this fix. All other validations already exist. Added new test cases corresponding to this validation, fixed all failing test cases because of this new validation and refactored some of the test case code.
Related Issues
Partially Resolves #2993
Check List
--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.