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.

Sign up for GitHub

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

Jump to bottom

do not allow non super admin users to undeploy hidden models #1981

Merged
merged 2 commits into from
Feb 2, 2024
Merged

do not allow non super admin users to undeploy hidden models #1981

merged 2 commits into from
Feb 2, 2024

Conversation

rbhavna
Copy link
Collaborator

@rbhavna rbhavna commented Feb 1, 2024

Description

do not allow non super admin users to undeploy hidden models

Issues Resolved

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --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.

@rbhavna
do not allow non super admin users to undeploy hidden models
4db6c81 
Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env February 1, 2024 21:55 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 21:55 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 21:55 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 21:55 — with GitHub Actions Inactive
ylwu-amzn
ylwu-amzn reviewed Feb 1, 2024
View reviewed changes
plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java Outdated
} else {
listener.onFailure(new IllegalArgumentException("No permission to undeploy model " + modelId));
}
}, listener::onFailure));
return;
} else {
// Only allow user undeploy one model if model access control enabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Add more comments explain why?

mingshl
mingshl reviewed Feb 1, 2024
View reviewed changes
plugin/src/main/java/org/opensearch/ml/action/undeploy/TransportUndeployModelsAction.java
throw new IllegalArgumentException("only support undeploy one model");
}

if (modelIds.length == 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

when there is only one model, why there is no need to check whether it's hidden model or not?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We do check it. All validations are done here when there is only one model

@rbhavna
add more comments
609fcf5 
Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env February 1, 2024 22:18 — with GitHub Actions Failure
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env February 1, 2024 22:18 — with GitHub Actions Failure
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env February 1, 2024 22:18 — with GitHub Actions Failure
@codecov Codecov
Copy link

codecov bot commented Feb 1, 2024 •
edited
Loading

Codecov Report

Attention: 43 lines in your changes are missing coverage. Please review.

Comparison is base (e98d6ec) 82.80% compared to head (609fcf5) 82.58%.
Report is 2 commits behind head on 2.x.

Files Patch % Lines
...action/undeploy/TransportUndeployModelsAction.java 10.41% 42 Missing and 1 partial ⚠️
Additional details and impacted files 
@@             Coverage Diff              @@
##                2.x    #1981      +/-   ##
============================================
- Coverage     82.80%   82.58%   -0.23%     
+ Complexity     5513     5502      -11     
============================================
  Files           533      533              
  Lines         22134    22167      +33     
  Branches       2253     2256       +3     
============================================
- Hits          18329    18306      -23     
- Misses         2881     2934      +53     
- Partials        924      927       +3
Flag Coverage Δ
ml-commons 82.58% <17.30%> (-0.23%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 22:38 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env February 1, 2024 22:38 — with GitHub Actions Failure
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env February 1, 2024 22:38 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 23:00 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 23:00 — with GitHub Actions Inactive
@rbhavna rbhavna had a problem deploying to ml-commons-cicd-env February 1, 2024 23:00 — with GitHub Actions Failure
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 23:18 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 23:18 — with GitHub Actions Inactive
@rbhavna rbhavna temporarily deployed to ml-commons-cicd-env February 1, 2024 23:18 — with GitHub Actions Inactive
@rbhavna rbhavna merged commit 4de949c into opensearch-project:2.x Feb 2, 2024
10 of 13 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Feb 2, 2024
@rbhavna @github-actions
do not allow non super admin users to undeploy hidden models (#1981)
e154b1d 
* do not allow non super admin users to undeploy hidden models

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>
(cherry picked from commit 4de949c)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Feb 2, 2024
Merged
rbhavna added a commit that referenced this pull request Feb 2, 2024
@opensearch-trigger-bot @rbhavna
do not allow non super admin users to undeploy hidden models (#1981) (#…
f63c4df 
…1983)

* do not allow non super admin users to undeploy hidden models

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>
(cherry picked from commit 4de949c)

Co-authored-by: Bhavana Ramaram <rbhavna@amazon.com>
austintlee pushed a commit to austintlee/ml-commons that referenced this pull request Mar 19, 2024
@opensearch-trigger-bot @rbhavna @austintlee
do not allow non super admin users to undeploy hidden models (opensea…
04c07d2 
…rch-project#1981) (opensearch-project#1983)

* do not allow non super admin users to undeploy hidden models

Signed-off-by: Bhavana Ramaram <rbhavna@amazon.com>
(cherry picked from commit 4de949c)

Co-authored-by: Bhavana Ramaram <rbhavna@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mingshl mingshl mingshl approved these changes

@ylwu-amzn ylwu-amzn ylwu-amzn approved these changes

@b4sjoo b4sjoo Awaiting requested review from b4sjoo b4sjoo is a code owner

@dhrubo-os dhrubo-os Awaiting requested review from dhrubo-os dhrubo-os is a code owner

@jngz-es jngz-es Awaiting requested review from jngz-es jngz-es is a code owner

@model-collapse model-collapse Awaiting requested review from model-collapse model-collapse is a code owner

@zane-neo zane-neo Awaiting requested review from zane-neo zane-neo is a code owner

@Zhangxunmt Zhangxunmt Awaiting requested review from Zhangxunmt Zhangxunmt is a code owner

@austintlee austintlee Awaiting requested review from austintlee austintlee is a code owner

@HenryL27 HenryL27 Awaiting requested review from HenryL27 HenryL27 is a code owner

Assignees
No one assigned
Labels
backport main
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rbhavna @ylwu-amzn @mingshl