Execlude trusted connector check for hidden model #3838
Conversation
Signed-off-by: Dhrubo Saha <[email protected]>
dhrubo-os
requested review from
b4sjoo,
mingshl,
jngz-es,
model-collapse,
rbhavna,
ylwu-amzn,
zane-neo,
Zhangxunmt,
austintlee,
HenryL27 and
xinyual
as code owners
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dhrubo-os
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Error
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dhrubo-os
had a problem deploying
to
ml-commons-cicd-env
GitHub Actions
Failure
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
|
This pr is only to skip url validation for inline connectors, correct? |
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
dhrubo-os
temporarily deployed
to
ml-commons-cicd-env
GitHub Actions
Inactive
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3838 +/- ##
=========================================
Coverage 77.99% 78.00%
- Complexity 7316 7318 +2
=========================================
Files 655 655
Lines 33032 33035 +3
Branches 3706 3707 +1
=========================================
+ Hits 25764 25768 +4
Misses 5681 5681
+ Partials 1587 1586 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Yeah. We support hidden model with inline connector. So I'm putting the validation only here for simplicity. Do you see any concern? |
|
No, it's OK to me. Thanks for confirming. |
| @@ -171,7 +172,8 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLRegi | |||
| "To upload custom model user needs to enable allow_registering_model_via_url settings. Otherwise please use OpenSearch pre-trained models." | |||
| ); | |||
| } | |||
| registerModelInput.setIsHidden(RestActionUtils.isSuperAdminUser(clusterService, client)); | |||
| boolean isSuperAdmin = isSuperAdminUserWrapper(clusterService, client); | |||
| registerModelInput.setIsHidden(isSuperAdmin); | |||
There was a problem hiding this comment.
Should we check hidden field in model configuration?
Is it possible super admin create non-hidden model?
There was a problem hiding this comment.
In current design this is not possible for super admin to create non-hidden model.
| @@ -171,7 +172,8 @@ protected void doExecute(Task task, ActionRequest request, ActionListener<MLRegi | |||
| "To upload custom model user needs to enable allow_registering_model_via_url settings. Otherwise please use OpenSearch pre-trained models." | |||
| ); | |||
| } | |||
| registerModelInput.setIsHidden(RestActionUtils.isSuperAdminUser(clusterService, client)); | |||
| boolean isSuperAdmin = isSuperAdminUserWrapper(clusterService, client); | |||
There was a problem hiding this comment.
This method for a single line adds complexity, additional method maintenance and reduced readability. this isn't typically considered best practice unless there's a specific testing requirement, but this static method is not covered in the UT at all.
There was a problem hiding this comment.
If you notice, I have mocked the behavior in both of the tests:
// Mock super admin check
doReturn(false).when(transportRegisterModelAction).isSuperAdminUserWrapper(any(), any());
Description
[Execlude trusted connector check for hidden model]
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
--signoff.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.